fix(app-shell,plugin-dashboard,console): send query options with their $, and gate the shape - #5946
Merged
Merged
Conversation
…r `$`, and gate the shape (#5458) `QueryParams` declares every query option `$`-prefixed and `convertQueryParams` copies exactly those keys, so an unprefixed spelling reaches no branch and is dropped — silently, and it type-checks because the type carries `[key: string]: any` for adapter-specific params. A dropped cap makes the read UNBOUNDED rather than truncated: the platform's GET list route has no default page size. Four live sites, all fixed: - `ObjectView` fetched the footer record count with `{ limit: 0 }` — the one that INVERTED, since `$top: 0` means "no records": "count only" became "fetch every row", on every mount of every list view. Now `$top: 0`, reading `total` only; the row-counting fallbacks are dropped rather than repointed, because an empty `data` after asking for zero rows says nothing about the object. - `AssignedUsersSection` — `{ …, limit: 1 }`, one line from three correct `$top` calls. - `DashboardFilterBar` — `fields` AND `top` in one literal, so a filter's option list read every row and every column of its source; it also read `records.items`, not a `QueryResult` member, so a real adapter yielded no options at all. Found by the new rule, not by the card. - `sdui-workbench-preview` — `{ top: 200 }` plus a `.records` misread, inside page-source metadata. New `object-ui/no-unprefixed-query-params` rejects the shape at write time: a known query-option name missing its `$` in the second argument of a `find`/`findOne` call. Narrow on purpose — a closed spelling list anchored to the call — because the index signature exists for adapter-specific params. The sibling `no-query-params-under-options` is unchanged and still gates its half. Part of #5458 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PQ3NihCHE9LUtHoGxo6A9f
Contributor
✅ Console Performance Budget
The eager closure is every chunk the entry reaches through static imports — what the browser fetches and parses before the app renders. The entry chunk on its own is a small fraction of it. 📦 Bundle Size Report
Size Limits
|
This was referenced Aug 24, 2026
os-warren
marked this pull request as ready for review
August 24, 2026 03:22
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #5458
Verified at
3a683ea— every run quoted below is that tree.The class
QueryParamsdeclares every query option$-prefixed andObjectStackAdapter.convertQueryParamsbuilds its outgoing options by copying exactly those keys. Any other key reaches no branch and is dropped: no throw, no warning. It type-checks becauseQueryParamscarries[key: string]: any, deliberately, so adapters can take adapter-specific params.For a dropped cap the consequence is an unbounded read, not a truncated one. Confirmed against the pinned client rather than assumed —
@objectstack/client@17.2.0emits the cap only when the caller supplied one:!= null, sotop: 0does reach the wire astop=0; an absenttopis what returns the whole match set.Call sites, not the adapter
Per the card's ruling, all fixes are at the call sites.
convertQueryParamsis unchanged — teaching it to honour unprefixed keys would widen the acceptance set the index signature exists to keep narrow.app-shellObjectView.tsx— the one that invertedIts comment says "Fetch record count for footer display", the handler reads
result.totalfirst, and the value feeds the footer'srecordCount. So the intent is unambiguous: give me the total, send no rows — and$top: 0is the correct spelling of exactly that.limit: 0was dropped, so the effect did the opposite of what it says: it downloaded every row of the object, on every mount and every refresh of every list view, to read one integer off the envelope.The row-counting fallbacks (
result.data.length,result.length) are removed rather than repointed, and that is the non-mechanical half of this fix. Once zero rows are requested, an emptydatameans "you asked for none", not "the object is empty" — keeping those arms would have turned a correct-but-expensive count into a confident0for any adapter that does not report a total.totalis now the only reader; with no total the footer line is omitted, which the render already supports (typeof recordCount === 'number'). For the ObjectStack adapter this changes nothing observable:normalizeQueryResultalways produces a numerictotal(resultObj.total ?? resultObj.count ?? records.length), so the first branch already won.app-shellAssignedUsersSection.tsx{ $filter: { name }, limit: 1 }→$top: 1. A single-record lookup by unique name, sitting one line from three correct$topcalls.plugin-dashboardDashboardFilterBar.tsx— a fourth site the card never namedFound by the new rule on its first repo-wide run, which is the argument for the rule in one site. It passed
fieldsandtopin one literal, so a filter's option list read every row and every column of its source object, under a comment describing it as a "client-side dedupe (top 200 records)". The same call then readrecords.items— not aQueryResultmember — so against a real adapter the fallback resolved to[]and the filter offered no options at all.$selectreaching the wire for the first time is a behaviour change, so the projection is deduped:valueField === labelFieldis the common case and a repeated entry would be a new thing to send.consolesdui-workbench-preview.tsx{ top: 200 }→$top: 200, andall.records→all.data. See the gate boundary below — this one is not reachable by the rule.The rule: a sibling, not a second predicate
no-unprefixed-query-paramsis new rather than an extension ofno-query-params-under-options, because the two need different anchors. The sibling's signature (a$-key underoptions) is unmistakable in any object literal, so it is context-free. Every name on this one's list —top,limit,filter,sort,select,count— is an ordinary object key elsewhere in this repo and carries no signal at all away from a finder call, so this one must be anchored to the call. They also must stay separately silenceable: oneeslint-disableshould not switch off both halves of the class.Narrow on two independent axes: a closed list of known query-option spellings (not "any unprefixed key", which would report the adapter-specific params the index signature exists for), and only the second argument of a
find/findOnecall.Array.prototype.findis excluded by its own signature — a predicate first argument — rather than by naming.fields → $selectis one entry beyond the card's enumerated list. It is the same kind of entry aslimit → $topandoffset → $skip(an alias spelling, not a literal missing$), and it earned its place on the liveDashboardFilterBarsite rather than on speculation. Flagged here for review since the card's list was explicit.The four properties, each as a run
(a) Fails on the live sites before the fix, passes after. Same three files, real repo config, before and after:
Two errors, not three — see the boundary below.
(b) False-positive control. The three correct neighbours in
AssignedUsersSection.tsx($top: 500L124,$top: 200L131,$top: 1000L151/171) produced zero reports in that same run; only L109 fired. They are pinned asvalidcases in the rule's test.(c) The already-gated half still fails.
no-query-params-under-optionsis unchanged, and the new test file re-runs it on both instances it was written for ({ options: { $top: 100 } }, and object-kanban's split spelling) as a cross-check that the two rules still divide the class and neither has swallowed the other.(d) Wired into the config CI runs — verified by making it fire, not by reading config. The before/after runs above are
npx eslintwith the repo's owneslint.config.js; the rule is registered ineslint-rules/index.jsand set toerrorin the same block as its sibling.scripts/__tests__/lint-workflow.test.tspasses (it imports the resolved flat config and assertsobject-ui/*error rules exist); no rule name was added to any of the three prose surfaces that test scans.Gate boundary: one site the rule cannot reach
sdui-workbench-preview.tsxholds its page source in a template literal — runtime page metadata, oneTemplateLiteraltoken to the parser, never aCallExpression. No AST rule can see into it, so that site was fixed by hand and is not gated. Confirmed by measurement: with the rule enabled repo-wide over the pre-fix tree it reported the other sites and zero findings in that file while its{ top: 200 }was live. Recorded with a suggested mechanism (extending the existingsdui-preview-page-source-*test family, which already glob-enumerates the harnesses and extracts theirsourcestrings) as #5944.Population census
The rule is
error-level and repo-wide, so it had to lint clean before landing. Enabled alone over the whole repo:The pre-fix run of the same scan is what surfaced
DashboardFilterBar.tsx.Verification
Repo-root vitest (package-scoped runs use a different config than CI), each named with its own result:
vitest run eslint-rules/vitest run scripts/__tests__/lint-workflow.test.ts eslint-rules/vitest run …/AssignedUsersSection.test.tsx packages/plugin-dashboard/vitest run packages/app-shell/src/views/ObjectView apps/console/pnpm --filter @object-ui/{app-shell,plugin-dashboard,console} type-checkDone, exit 0The type-check used the script's real name
type-check; the hyphenless spelling matches zero scripts and exits 0 without running anything. All three package names are echoed in the log, so the match was non-empty.Node gates, each by name with its own verdict line:
Reverse verification
The
records.items → datafix has a discriminating test, proven by mutation rather than asserted. Mutation confirmed on disk before the run (records?.itemscount 1,records?.datacount 0,git diff --statnon-empty), restored by anEXITtrap and verified byte-identical afterwards. No rebuild needed and none claimed: the test imports../DashboardFilterBarby relative path, so vitest transforms the source and nodistis involved.Predicted direction — the new test alone goes red — and that is what happened: 1 failed, 13 passed, the failure being the
QueryResultenvelope case timing out onfindByRole('option'), i.e. the dropdown opening empty. Every other case in that file mocks a bare array and takes theArray.isArrayarm, which is why the defect had survived there.Fixture triage
DashboardFilterBar.options.test.tsxpinned the dead spelling —expect.objectContaining({ top: 200 })— so it passed while asserting the defect. It now pins the exact params object ({ $select: ['industry'], $top: 200 }); a partial matcher is what let the bare key sit there looking asserted.Scope notes
packages/plugin-dashboard/was not in the dispatched surface. It is included because the rule cannot land without it: anerror-level rule with a live violation turns CI red, and everyobject-ui/*ratchet ineslint.config.jsfollows the same convention of converting its sites first. Same defect class, mechanical fix, no open PR touches that file.content/docs/guide/react-pages.md(already corrected —:231uses$filterand:243documents the convention in prose, so it is an ally, not a target);skills/**;preview/DraftChangesPanel.tsx;apps/console/src/__tests__/public-block-binding-reach.test.tsx.skills/objectui/guides/data-integration.mdis worse than the card recorded — beyondreturn result.recordsat :275, the same snippet dropsfilter,sortandlimit, and theQueryResultsketch at :78 declaresrecords?: T[]and omitsdataentirely, which is the root of the misread. Published skill surface, left for the PM.no-unprefixed-query-paramscannot seefind()calls inside page-source template literals, so one of the four #5458 sites has no mechanical gate #5944 (the template-literal gate gap),asArrayhelpers read three result shapes, two of which are notQueryResultmembers — a tolerant reader standing in for the contract #5945 (asArrayhelpers reading two non-contract result shapes ahead ofdata).Generated by Claude Code
Generated by Claude Code