fix(plugin-grid): skip and diagnose an unusable bulkActionDefs member instead of crashing the selection bar - #8741
Merged
Conversation
…er instead of crashing the selection bar (objectui#8730)
`bulkActions` and `bulkActionDefs` are one affordance in two vocabularies —
bare action NAMES resolved against `objectDef.actions`, and full
`BulkActionDef` OBJECTS used as authored — and nothing refused a member
written in the other one: both keys are registered `type: 'array'` with no
`of`, both spec rows are `z.array(z.unknown())`, and a JSON view is invisible
to `tsc`.
Writing a bare name into `bulkActionDefs` crashed the render. The string
travelled into the authored list untouched, `BulkActionBar` rendered a button
for it, and `def.label ?? formatActionLabel(def.name)` threw `TypeError:
Cannot read properties of undefined (reading 'replace')` — so the author's
first multi-row selection lost the whole selection bar, including its count,
its Clear button and every well-formed sibling def. `key={def.name}` was
`undefined` as well.
`resolveBulkActions` now skips any member that is not an object carrying a
non-empty string `name`. "Usable" is defined by what the renderer reads, not
by a fresh opinion: `name` is both the React `key` and `formatActionLabel`'s
argument, so one test covers the reported bare string and, identically,
`null`, a number, `{}` and `{ name: '' }`. The guard sits at the single point
where the authored array becomes the list the bar maps over, so the key and
the label are read off the same validated def, and the clean case still
returns the authored array by reference.
The skip is not silent: `ObjectGrid` reports it once per authored array
through the channel it already owns for "you declared it, the renderer
dropped it", naming the block, the index, what was seen and what to write
instead — including that a bare name belongs in `bulkActions`.
Not a coercion, deliberately: `'approve'` is not lifted into `{ name:
'approve' }` and resolved the way `bulkActions` is. That would make the two
vocabularies interchangeable, which is a product change to what a
`bulkActionDefs` member means (objectui#3002 / objectui#3139), not a crash
fix. The mirror direction (`bulkActions: [{ name: 'approve' }]`) keeps its
existing silent skip.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01YBWFb5YgMU5dw8p2VKj16S
…pins `ReturnType<typeof vi.spyOn>` erases the call-log element type, so both helpers took `args` implicitly `any` and `tsc -p tsconfig.test.json` refused the file with four TS7006s. The package's `type-check` is what caught it — the file was green under vitest either way, which is exactly the gap between "the tests pass" and "the tests are checked". A local structural `ConsoleSpy` reads only what these helpers use (the call log) and stays true for both `console.warn` and `console.error` spies. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YBWFb5YgMU5dw8p2VKj16S
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-justin
marked this pull request as ready for review
September 9, 2026 02:47
os-justin
enabled auto-merge
September 9, 2026 02:47
Collaborator
Author
|
Contract review — accepted, flipped out of draft, auto-merge armed. 25 of 33 green with shards, Type Check and Lint still running; nothing failed. First, a correction that is mine. My dispatch instructed you to rewrite The way you established that is the part worth keeping: an Three failure modes, not one. The card implies a single crash; you measured three:
So for the last one this is a behaviour change rather than a crash removal, and saying that plainly is better than letting "fixes the crash" quietly cover it. The guard site is the right one and the reasoning is checkable. ⭐ The discriminating pair is exactly what the ruling asked for, and the cross-check is better than the legs. "Skip everything" reddens 6/7/8 while 1–5 stay green — which is why rows 6, 7 and 8 had to exist, since the unusable-class rows alone are passed by an implementation that skips everything. "Skip nothing" reddens 1,2,3,4,5,6,8,9. And that mutation's red set is byte-identical to the base-tree red set — independent evidence that the mutation reproduced the reported defect rather than a new one. I did not ask for that check; it is the strongest line in the report. The diagnostic is an assertion, both ways. It fires naming the block, the index, the member and the other key where a bare name belongs; it does not fire for a clean list; and the no-fire leg reddens under "skip everything", so "a warning that fires always" is refused by measurement rather than by intent. Routing it through the channel Direction one stayed out, with the measurement that settles it: nothing on
Noted and agreed on not filing: Generated by Claude Code |
os-warren
pushed a commit
that referenced
this pull request
Sep 9, 2026
…ose contract that landed Row 5 pinned objectui#8730's crash as the CURRENT shape, with the issue filed from it, so that landing the fix would RED the row rather than leave it describing nothing. The fix landed — PR #8741, commit `baf3776a`, objectui#8730 closed completed — this branch's base predates it, and the merge-queue candidate went red on exactly that row. This is the rewrite the row was built to force. The new contract: a `bulkActionDefs` member that is not an object with a non-empty string `name` is SKIPPED by `resolveBulkActions`, and reported once per authored array through `ObjectGrid`'s existing "you declared it, the renderer dropped it" `console.warn` channel, naming the block, the index, what was seen and what to write instead. The row keeps its discriminating job — a bare name is not this key's vocabulary — and asserts three things a bare "does not throw" would not: - SKIPPED, not coerced. `'approve'` is a real declared object action, so a read site that lifted it into `{ name: 'approve' }` would render a button labelled "Approve the invoice". An exact ordered census refuses that, and the absent label is asserted by name rather than implied. - EXACTLY ONE MEMBER WIDE. A well-formed sibling travels with the bad member, so "the bar survives" cannot be satisfied by a renderer that skips everything and produces an empty bar. - DIAGNOSED. One line, addressing the member by index and naming `bulkActions` as where a bare name belongs — with row 4 rewritten into its NO-FIRE leg, since a warning that fires for every authored array is worth as little as one that never fires. Class-level coverage of the fix (every unusable member kind, survivor order, referential identity) is `bulkActionDefsUnusableMember-8730.test.tsx`; this file keeps the member-VOCABULARY half it was written for. The file docblock now records the asymmetry both directions still have — direction one silent, direction two diagnosed — instead of the pre-fix "silence on both". Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Jmxdo7bmeqCQHLSfmLVX9w
akarma-synetal
pushed a commit
to akarma-synetal/objectui
that referenced
this pull request
Sep 9, 2026
… exportOptions / bulkActions / bulkActionDefs (objectui#8071 slice 1) (objectstack-ai#8737) * test(spec-parity): pin the member shapes of object-form.fields and object-grid's three action/export keys objectui#8071's first declared slice: convert four of the 62 exempted array/object-armed inputs into named per-block member pins, delete their exemptions and lower the ratchet with them (62 -> 58). The criterion objectui#8068 set is that a pin constrains the shape the RENDERER reads, never a restatement of the registration. Measured per key: - object-form.fields — SimpleObjectForm's own `fieldsToShow` loop reads members as BARE FIELD NAMES against the object schema. New behavioural file, because the near-miss test the card named (sectionFields.spec-parity.test.ts) pins the neighbouring `sections[].fields`, whose member is the spec FormFieldSchema object keyed on `field` — an entry the top-level key drops in silence. - object-grid.bulkActions / .bulkActionDefs — two vocabularies, one selection bar: bare action names promoted against objectDef.actions, versus full BulkActionDefs left as authored. Both cross-substitutions are pinned. - object-grid.exportOptions — the existing read-set scanner grows a third direction: the read set against the object-grid REGISTRATION's own member enumeration. Two findings came out of writing them, filed rather than fixed here: objectui#8730 (a bare-string member of bulkActionDefs throws in render and takes the selection bar down) and objectui#8731 (the registration documents four exportOptions member keys while the renderer reads five — `streaming` is honoured and unadvertised). Refs objectstack-ai#8071 (object-form.fields, object-grid.exportOptions, object-grid.bulkActions, object-grid.bulkActionDefs) Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Jmxdo7bmeqCQHLSfmLVX9w * test(plugin-form): drop the unused `screen` import the object-ui/no-unused-imports rule refused Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Jmxdo7bmeqCQHLSfmLVX9w * test(plugin-grid): move objectui#8071's row 5 onto the skip-and-diagnose contract that landed Row 5 pinned objectui#8730's crash as the CURRENT shape, with the issue filed from it, so that landing the fix would RED the row rather than leave it describing nothing. The fix landed — PR objectstack-ai#8741, commit `baf3776a`, objectui#8730 closed completed — this branch's base predates it, and the merge-queue candidate went red on exactly that row. This is the rewrite the row was built to force. The new contract: a `bulkActionDefs` member that is not an object with a non-empty string `name` is SKIPPED by `resolveBulkActions`, and reported once per authored array through `ObjectGrid`'s existing "you declared it, the renderer dropped it" `console.warn` channel, naming the block, the index, what was seen and what to write instead. The row keeps its discriminating job — a bare name is not this key's vocabulary — and asserts three things a bare "does not throw" would not: - SKIPPED, not coerced. `'approve'` is a real declared object action, so a read site that lifted it into `{ name: 'approve' }` would render a button labelled "Approve the invoice". An exact ordered census refuses that, and the absent label is asserted by name rather than implied. - EXACTLY ONE MEMBER WIDE. A well-formed sibling travels with the bad member, so "the bar survives" cannot be satisfied by a renderer that skips everything and produces an empty bar. - DIAGNOSED. One line, addressing the member by index and naming `bulkActions` as where a bare name belongs — with row 4 rewritten into its NO-FIRE leg, since a warning that fires for every authored array is worth as little as one that never fires. Class-level coverage of the fix (every unusable member kind, survivor order, referential identity) is `bulkActionDefsUnusableMember-8730.test.tsx`; this file keeps the member-VOCABULARY half it was written for. The file docblock now records the asymmetry both directions still have — direction one silent, direction two diagnosed — instead of the pre-fix "silence on both". Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Jmxdo7bmeqCQHLSfmLVX9w * test(plugin-grid): name row 1 as the reachability control for row 5's absent-label assertion `expect(queryByText('Approve the invoice')).not.toBeInTheDocument()` is a negative, and a negative over a string nothing ever produces asserts nothing. Row 1 renders exactly that string — it is the object action's own label, reached by resolving a member as a NAME in the key whose vocabulary that is — so the comment now points at it. The absence in row 5 is a measured difference between the two keys, not an accident of spelling. Comment only; no assertion changed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Jmxdo7bmeqCQHLSfmLVX9w --------- Co-authored-by: Claude <noreply@anthropic.com>
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 #8730
object-gridhas two vocabularies for one affordance, and nothing refused a memberwritten in the wrong one.
bulkActionsmembers are bare action NAMES resolved againstobjectDef.actions;bulkActionDefsmembers are fullBulkActionDefOBJECTS used asauthored. Both keys are registered
type: 'array'with noof, both spec rows arez.array(z.unknown()), and a JSON view is invisible totsc.Writing a bare name into
bulkActionDefsdid not fail quietly — it crashed the render:The author's first multi-row selection lost the entire selection bar — count, Clear and
every well-formed sibling def with it.
What changed
resolveBulkActionsskips anybulkActionDefsmember that is not an object carrying anon-empty string
name, andObjectGridreports the skip once, by address."Usable" is defined by what the renderer reads, not by a fresh opinion about what a
def ought to carry:
BulkActionBarusesdef.nametwice — as the Reactkeyof thebutton it maps to, and as
formatActionLabel's argument when the def declares nolabel. Every otherBulkActionDefkey is optional at the read site, sonameis thewhole predicate — and that one test covers the reported bare string plus, identically,
null, a number,{}and{ name: '' }.The guard site.
resolveBulkActions'srawAuthoredis the single point where theauthored array becomes the list the bar maps over: its
defsreturn flows toObjectGrid'sbulkActionDefs, toBulkActionBar'sactionDefs, topermittedDefs,to the
.map()that readskey={def.name}anddef.label ?? formatActionLabel(def.name).Guarding there means the key and the label are read off the same validated def, and
BulkActionBarkeeps readingdef.nameunconditionally because by that line it can.There is one predicate with two readers — the filter and the message — not a check
scattered across the render path.
Referential identity is preserved (
everybeforefilter): thedefscontractpromises the authored array by reference when nothing folds in, and an always-allocating
filter would break that for every clean view.
Not a coercion, deliberately.
'approve'is not lifted into{ name: 'approve' }and resolved the way
bulkActionsis. That would make the two vocabulariesinterchangeable — a product change to what a
bulkActionDefsmember means (objectui#3002/ objectui#3139 made them distinct on purpose), not a crash fix.
The diagnostic rides the channel
ObjectGridalready owns for "you declared it, therenderer dropped it" (one
console.warnprefixed[ObjectUI] ObjectGrid TOPIC:, auseEffectkeyed on the schema slice — the same shape as the columns diagnostic and theexport-format warning, not a third one beside them):
Out of scope, by the dispatch ruling: the mirror direction
(
bulkActions: [{ name: 'approve' }], stepped over byresolveBulkActions'stypeof name !== 'string'guard) keeps its existing silent skip. Row 9 of the new pinasserts that silence, so a diagnostic leaking into it would be caught.
Verification — by content
Everything below is quoted from the vitest JSON reporter (never the text reporter: a
TypeErrorat module scope reads as a suite death, not a failing assertion), run fromthe repo root with file paths.
Reproduction, on the base tree (
a9bc02996, source blobs proven identical toHEAD:bygit hash-objectbefore the run): 8 of the 10 new pins RED, including thereported stack verbatim. Three distinct pre-fix failure modes were measured, not one:
'approve'/42/{}threw informatActionLabel;nullthrew one frame earlier atkey={def.name}(Cannot read properties of null (reading 'name')); and{ name: '' }did not throw at all — it rendered a nameless, unlabelled button
(
expected [ 'bulk-action-' ] to deeply equal []).The discriminating axis. "Skip the bad member" is also satisfied by an implementation
that skips everything, so both caricatures were run as mutations of the committed fix,
each with an
EXIT INT TERMtrap, each proven to have reached disk (deleted-anchorcount 0, injected count 1, blob differs from
HEAD:) and each restored withgit checkout HEAD -- ABSOLUTE_PATHproven by an emptygit diff HEADand a matchinggit hash-object:isUsable...forcedfalse(skip everything)true(skip nothing)expected [] to equal [ 'bulk-action-archive', … ])defsby referenceexpected [ Array(1) ] to equal [])Rows 6, 7 and 8's no-fire leg are what "skip everything" cannot pass; rows 1-5, 8's fire
leg and 9 are what "skip nothing" cannot pass. Rows 7 and 10 are the untouched-behaviour
anchors, and are green under "skip nothing" precisely because that mutation is the base
behaviour — its red set is byte-identical to the base-tree red set, which is an
independent check that the mutation reproduced the defect rather than a new one.
Green (all at
b9b0abeab, a clean tree):packages/plugin-grid/: 347 files, 1055 tests, 1055 passed, 0 failedpackages/plugin-view/ packages/plugin-designer/ packages/plugin-list/ examples/schema-catalog/: 546 files, 3488 tests, 3488 passed, 0 failedpackages/app-shell/: 2156 files, 6406 tests, 6405 passed, 0 failed (1 skipped)turbo run type-check --filter=@object-ui/plugin-grid(builds the dependency closurefirst): exit 0. It earned its place — it caught four
TS7006s in the new pins thatvitest was green through; fixed in the second commit.
node scripts/check-control-bytes.mjs:OK (scanned 6922 tracked text file(s)); plus adirect
grep -naPcontrol-byte scan of the four changed files, zero hits.node scripts/check-changeset-presence.mjs: exit 0, declares the changeset.node scripts/check-governed-queue-guard.mjs --teston all four paths:NOT GOVERNED - 4 path(s) checked against 5 governed surface(s); none matched.Lint, narrowed and declared.
eslint packages/plugin-grid --format json— exit 0,155 files linted (the count read from the JSON output, the population from eslint's
own flat config, not from a guess), 0 errors. The 99 files carrying warnings are
pre-existing
no-explicit-any/react-refreshnoise; the three warnings on the new pinfile are the
as anyschema casts every sibling grid test in this package carries. Therest of the farm is CI's: this repo's root
lintisturbo run lintper package.The narrowing is safe to state because this config is not type-aware — no
projectServiceand noparserOptions.projectanywhere ineslint.config.js— so thisdiff cannot move the verdict on any file it did not touch.
NOT MEASURED, deliberately:
check:doc-snippets,check:doc-examples,check:readme-exportsandcheck:eager-closurewere not run. They need a fully builttree and
resolveBulkActionsis not exported frompackages/plugin-grid/src/index.tsx,so the two new exports are package-internal and reach no published surface. CI owns them.
A premise in the card that measurement contradicted
The dispatch says
packages/plugin-grid/src/__tests__/bulkActionMembers-8071.test.tsx"currently asserts the render rejects with that exact
TypeError" and must be rewrittenin this same commit. That file is not on
main. It exists only on the unmergedbranch
origin/claude/issue-8071-member-pins-near-miss-four(commite4b19de6f), whichgit merge-base --is-ancestorreports is not an ancestor ofmain— a reading confirmedagainst a shallow-clone false negative two ways:
git rev-parse --is-shallow-repositoryanswersfalse, and a control commit known to be in thishistory answers exit 0 on the same check.
So there is nothing here to rewrite, and creating that path on this branch would only
manufacture an add/add conflict with the other PR. Instead:
not direction one's silent drop. Before this change that distinction was
crash-vs-silence; it is now diagnostic-vs-silence, and both legs are asserted together
so they cannot drift apart.
there. That is visible rather than silent, but it is a real cross-PR interaction and
should not be discovered by the merge queue.
Its row 2 was also the measurement behind leaving direction one alone: it asserts
expect(warn).not.toHaveBeenCalled()forbulkActions: [{ name: 'approve' }], i.e. anin-flight pin does assert the current silence. Nothing on
mainasserts it either way.Direction one is therefore untouched, as the ruling ordered, and row 9 now pins that
silence on
maintoo.Generated by Claude Code