Uh oh!
There was an error while loading. Please reload this page.
fix(plugin-grid): a null entry in grouping.fields[] no longer crashes the grid - #7346
Merged
Merged
Conversation
…es the grid `ObjectGrid`'s `groupValueFormatter` memo walked `schema.grouping.fields` and read `gf.field` off every entry with no guard, so a single `null` hole threw `TypeError: Cannot read properties of null (reading 'field')` out of render — the whole grid, gone, before any projection was built. `useGroupedData` is a second dereference site of the same list (`fields[depth]` then `.field` / `.order` / `.collapsed`), so guarding the memo alone would only have moved the crash one call downstream. Both sites now read one normalized entry list, `usableGroupingFields`. Its admission rule is deliberately the harvester's: an entry is usable when it is an object carrying a non-empty string `field` — exactly what `collectGroupingFieldRefs` contributes to the query projection. Keeping the two sets equal is load-bearing; an entry the grid grouped by but the projection ignored would read `undefined` on every row and bucket every record into one `(empty)` group, which is the silent wrong answer objectui#7179 closed. One bad entry is dropped, not the grouping: the usable levels still group. Measured before fixing, as the card asked: author-time validation ALREADY refuses a null entry (`GroupingConfigSchema` types `fields` as an array of `$strict` objects; objectui's `ListViewSchema` inherits it by reference), so this is a defensive guard rather than a validation gap. The crash stays live because nothing on the render path runs that validator — `ObjectGrid` reads `schema.grouping` straight off its props and `validateSchema` is structural. Both halves of that measurement are pinned in the new test file. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NRRumy89BYdW9ogbcdHTho
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
|
os-litant
commented
Sep 2, 2026
CollaboratorAuthor
CI red on Generated by Claude Code |
…ty per name `one-authority-per-exported-name-6273` went red: the alias introduced by the null-entry guard published `GroupingFieldEntry`, a name `@object-ui/components` already declares at `custom/grouping-editor.tsx:14`. The two are genuinely different shapes, so the remedy is the gate's rename branch rather than re-pointing one at the other: the components interface is the grouping EDITOR's fully-populated value, with `order` and `collapsed` REQUIRED, while this one is `z.input` of the spec's `GroupingConfig`, where both carry defaults and are therefore optional. Collapsing them would make one of the two lie. Renamed to `UsableGroupingField` — free repo-wide, and it says what it is: an entry that passed `usableGroupingFields`. The components interface is untouched; it keeps the name it already owned. A comment at the declaration records why the two names differ, so the next reader does not "unify" them. No behaviour change — the rename is type-level only, and the grid pin is unaffected. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NRRumy89BYdW9ogbcdHTho
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
|
os-litant
marked this pull request as ready for review
September 2, 2026 13:17
Uh oh!
There was an error while loading. Please reload this page.
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 freeto 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#7217
Branch forked from
origin/mainate75f4c986. Round-1 numbers were measured on5eb7a8138; the Round 2 section at the end records the CI fix and what was re-run on the final commit410823207.Premise re-verified on the base sha
The card's claim holds, unchanged, at the fork point and on current
origin/main:packages/plugin-grid/src/ObjectGrid.tsx:2044-2045—for (const gf of grouping.fields) { const fieldName = gf.field;with no guard.packages/plugin-grid/src/useGroupedData.ts:251,257,260,268,278,290— the SECOND dereference site:const f = fields[depth]and thenf.field/f.order/f.collapsed.origin/mainmoved to2956d7af8while this ran.git log e75f4c986..origin/main -- packages/plugin-grid packages/core packages/typesshows PR #7284 and PR #7339 landed there, but both target lines are byte-identical and at the same line numbers on currentorigin/main, so the fork point was kept and the merge queue rebuilds.The fix
Both sites now read ONE normalized entry list,
usableGroupingFields, exported fromuseGroupedData.ts. An entry is usable when it is an object carrying a non-empty stringfield.That admission rule is deliberately the harvester's — exactly what
collectGroupingFieldRefs(packages/core/src/utils/grouping-fields.ts) contributes to the query projection. Keeping the two sets equal is load-bearing, not tidiness: an entry the grid grouped by but the projection ignored would readundefinedon every row and bucket every record into one(empty)group, which is the silent wrong answer objectui#7179 addressed. This is a defensive normalizer, not a lenient alias — no off-spec spelling is taught to mean anything; unusable entries are dropped, never coerced. And one bad entry drops the entry, not the grouping: the usable levels still group.Routing the memo through
collectGroupingFieldRefsitself was measured and rejected — see the table below.Reachability measurement (the card's "Measure which before fixing")
GroupingConfigSchematypesfieldsas an array of$strictobjects.safeParse({ fields: [null] })fails withinvalid_typeat pathfields.0. Declared in@objectstack/spec(view.zodd.ts lines 388-409); reached in this repo as@objectstack/spec/ui.ListViewSchemainherit that refusal?packages/types/src/zod/objectql.zod.ts:427imports the spec fields by reference viaspecFieldsExcept(SpecListViewSchema.shape, LIST_VIEW_LOCAL_OVERRIDES), andgroupingis NOT inLIST_VIEW_LOCAL_OVERRIDES(objectql.zod.ts:310-325), so the entry shape arrives with it.git grep -n grouping packages/types/src/zod/hits only comments (objectql.zod.ts:382,410). The shape arrives by reference, never restated. The PM's assumption 4 is therefore half right: nothing is re-declared locally, but a refusing validator does exist.schema.grouping?safeParse/validateSchemacall anywhere inpackages/plugin-grid/src/**outside tests.@object-ui/core'svalidateSchema(packages/core/src/validation/schema-validator.ts:458) runs base + retired-node-type + form + children only;packages/core/src/validation/contains zero occurrences ofgrouping— matching PR #7339's finding for this key too.ObjectGridreadsschema.groupingstraight off its props.GroupingEditor.writeFields(packages/components/src/custom/grouping-editor.tsx:76-158) only ever builds object literals ({ ...g, field: e.target.value },{ field, order, collapsed }) and removes with.filter(), which cannot leave a hole.useGroupReorder.tsreorders rendered GROUP KEYS, nevergrouping.fields[]entries, so it is not a writer at all.collectGroupingFieldRefsthe whole fix?GroupingFieldSchemaentry carriesorderandcollapsedbesidesfield. The harvester answers with field NAMES, so routing the memo through it would lose exactly the two propertiesuseGroupedDataneeds for sort order and default-collapsed state. Hence an entry-level normalizer that keeps the entry object.⇒ This is a defensive guard, not a validation gap — but the crash stays live, because nothing on the render path runs the validator that refuses it. A runtime-composed, generated, or agent-written schema reaches the memo unparsed. Both halves of this measurement are pinned in the new test file rather than left as prose.
Not in this PR (for the PM)
There is nothing to file on the accept set: author-time validation already refuses a null entry, so no published validator's accept set needs to move and Clause-② stays "no".
packages/types/src/**is untouched.The measurement did surface one separate defect, filed as objectui#7347 and deliberately not touched here: a grouping field name with surrounding whitespace is accepted by
GroupingFieldSchema(a barez.ZodString), is trimmed bycollectGroupingFieldRefson its way into$select, but is used untrimmed as the bucket key by both grid sites — so every row readsundefinedand lands in one(empty)group. Measured: specsafeParseaccepts{ field: " business_unit " }; the harvester yields["business_unit"]; the grid buckets by the raw padded key. It is the same silent-wrong-answer class as objectui#7179, and unlike the null entry it survives validation. The guard in this PR admits such an entry (itsfieldis a non-empty string), leaving today's behaviour exactly as it is onmain; normalizing the value as well as the admission set would be a behaviour change beyond this card, on a hot file.Test evidence
New pin:
packages/plugin-grid/src/__tests__/groupingNullEntry-7217.test.tsx(6 tests — 4 crash pins, 2 reachability measurements). The 7179 pin's narrowed malformed-block fixture was left exactly as it is.RED first, on the unmodified tree:
The 2 that passed on the unmodified tree are the reachability measurements — they are about the schema, not the guard, and are expected green in both directions.
Ablation, one site at a time
Each leg: mutate; prove the mutation landed on disk by counting the injected and the removed text (an edit tool's exit code proves nothing); run; restore with
git checkout HEAD --naming the file by ABSOLUTE path; prove the restore by blob hash against the HEAD blob plus an emptygit diff HEAD. The script carriedtrap restore EXIT INT TERMthroughout.No build leg applies: the pin imports
../ObjectGridrelatively and the root vitest config aliases@object-ui/*to each package'ssrc, so both mutation targets are read from source with nodist/in the path.ObjectGridmemo guard (hook guard stays)for (const gf of grouping.fields) {x1, removedfor (const gf of groupingFields) {x0Tests 4 failed | 2 passed (6), first errorTypeError: Cannot read properties of null (reading 'field')b8053e5e91d98da07cd30525417411b1c57c0d93== HEAD blob,git diff HEADemptyuseGroupedDataguard (memo guard stays)const fields = config?.fields;x1, removed theuseMemo(() => usableGroupingFields(rawFields), …)line x0Tests 4 failed | 2 passed (6), first errorTypeError: Cannot read properties of null (reading 'field')98d98f05bdcf27510ff6d9c9f70229a13630b2f5== HEAD blob,git diff HEADemptyBoth sites are independently load-bearing: guarding either one alone leaves all four crash pins red.
Verification table (round 1, on
5eb7a8138)Every heavy run went through the shared verification lock (
os-verify-lock.sh, slotdev-7217); the verdict quoted is the line the gate or the lock printed, never a bare$?.pnpm exec vitest run packages/plugin-grid/src/__tests__/groupingNullEntry-7217.test.tsxTest Files 1 passed (1)/Tests 6 passed (6)·VERDICT command-exit 0pnpm exec vitest run packages/plugin-grid/Test Files 110 passed (110)/Tests 999 passed (999)·VERDICT command-exit 0pnpm exec vitest run packages/plugin-list/Test Files 62 passed (62)/Tests 787 passed (787)·VERDICT command-exit 0pnpm --workspace-concurrency=2 --filter '@object-ui/plugin-grid^...' buildVERDICT command-exit 0pnpm --filter @object-ui/plugin-grid run type-check(tsc --noEmit && tsc -p tsconfig.test.json)VERDICT command-exit 0eslint .)pnpm exec eslint . --format jsoninpackages/plugin-gridVERDICT command-exit 0node scripts/check-changeset-presence.mjs✅ 3 source file(s) of 1 released package(s) changed, and this change declares 1 changeset(s): .changeset/7217-grouping-null-entry-guard.md.node scripts/check-changeset-no-major.mjs✅ No changeset declares a 'major' bump.node scripts/check-control-bytes.mjs✅ check-control-bytes: OK (scanned 6037 tracked text file(s); skipped 85 binary).node scripts/check-phantom-dependencies.mjsnode scripts/check-package-self-import.mjs✅ No package names itself inside its own src/.node scripts/check-vi-mock-specifiers.mjs✅ check-vi-mock-specifiers: OKnode scripts/check-vi-mock-inherit.mjs✅ check-vi-mock-inherit: OKnode scripts/pm/check-governed-merges.mjs --test …(final file list)✅ NOT governed — ordinary queue landing applies to a PR with exactly this file list.(0 of 4 paths hit the register)Type-check actually covers the new test file
tsc --noEmitalone would have said nothing about it.tsc -p tsconfig.test.json --listFiles | grep -c groupingNullEntry-7217= 1, so the new test file is genuinely inside the type-check program rather than excluded and silently unmeasured.Lint delta, against the fork-point blobs
Baseline taken by checking the two modified files out at
e75f4c986into the same paths (so eslint's config resolution is identical), proven landed on disk before measuring (usableGroupingFieldsoccurrences = 0), then restored and proven by blob hash:e75f4c986)ObjectGrid.tsxuseGroupedData.tsgroupingNullEntry-7217.test.tsxThe one added warning is
@typescript-eslint/no-explicit-anyonconst schema: any = {— the same idiom every sibling grid test uses (groupedBooleanLabel.test.tsx,groupingProjection-7179.test.tsx), and deliberate here: the fixture is intentionally off-spec, so a typed shape would refuse the null entry the test exists to feed in.Round 2 — the CI gate this missed, and the fix
Test (shard 4/4)went red on5eb7a8138inscripts/__tests__/one-authority-per-exported-name-6273.test.ts(objectui#6273, ruling objectui#6172). Verbatim:Round 1's alias published
GroupingFieldEntry, a name@object-ui/componentsalready owns.The two are genuinely different shapes, so this took the gate's rename branch rather than re-pointing one at the other:
@object-ui/componentsGroupingFieldEntryz.inputofGroupingConfig)order'asc' | 'desc'collapsedbooleanCollapsing them would have made one of the two lie, so each keeps its own name. Renamed to
UsableGroupingField— verified free acrosspackages,appsandscriptsbefore choosing — which also says what it is: an entry that passedusableGroupingFields. The components interface is untouched and keeps the name it already owned; nothing outsideuseGroupedData.tsreferenced the alias. A comment at the declaration records why the two names differ, so the next reader does not "unify" them.KNOWN_COLLISIONSwas NOT touched — it is shrink-only.Type-level only, no behaviour change.
Reproduced RED locally first, then green, on the final commit
410823207:Tests 1 failed | 10 passed (11)·VERDICT command-exit 1— sameGroupingFieldEntry — a NEW colliding namemessage as CITest Files 2 passed (2)/Tests 17 passed (17)·VERDICT command-exit 0scripts/meta-gate familyTest Files 97 passed (97)/Tests 2738 passed (2738)·VERDICT command-exit 0Test Files 110 passed (110)/Tests 999 passed (999)·VERDICT command-exit 0VERDICT command-exit 0useGroupedData.tsVERDICT command-exit 0✅ check-control-bytes: OK (scanned 6037 tracked text file(s); skipped 85 binary).Round 1's local gate derivation was package-scoped — the touched package's suites, type-check and
eslint ., plus thecheck:*scripts whose subject matter the diff touched. It never asked whether a repo-wide gate inscripts/__tests__/reads the diff, and this one does: it scans every exported type name in the monorepo, so adding one exported type anywhere can redden it. Round 2 therefore ran all 97 of those files rather than only the one that failed, which is the derivation round 1 should have made.Hot-file constraint (PR #7284)
Honoured.
git diff -U0onObjectGrid.tsxreports exactly three hunk headers:@@ -43 +43 @@,@@ -2037,2 +2037,9 @@and@@ -2044 +2051 @@— the import line 43 plus the grouping memo region. Line 39 (the@object-ui/coreimport) and thefieldsToShowdefault-column synthesis at roughly 2788-2810 are untouched — no new@object-ui/coreexport was needed, becauseusableGroupingFieldslives inplugin-grid's ownuseGroupedData.tsand rides the existing line-43./useGroupedDataimport. Round 2 touched onlyuseGroupedData.ts.packages/core/src/utils/grouping-fields.tswas READ as the route candidate and deliberately not edited: the harvester is already correct, and its name-only return is why it cannot be the whole fix.🤖 Generated with Claude Code
https://claude.ai/code/session_01NRRumy89BYdW9ogbcdHTho