Uh oh!
There was an error while loading. Please reload this page.
fix(app-shell): a WidgetContext option catalog carries its own load state - #5355
Conversation
…tate (#5228) Each option catalog on `WidgetContext` (`objectNames`, `objectFields`, `objectViews`, `objectActions`) becomes the four-arm `LoadState` the loaders already produce, instead of a plain array with the fault parked on a separate `catalogErrors` record and a `*Loading` flag beside it. The old shape let a FAILED load reach a picker as `[]`, byte-identical to a load that completed and found nothing, with the requirement to consult the failure channel living in a doc comment. `context?.objectFields ?? []` no longer compiles, and reading the list goes through `offeredOptions`, whose parameter type excludes the failure arm — so a call site that has not decided what a failure looks like does not compile. Fixes one live instance found by the compiler: the View variant inspector, a second host of `WidgetContext`, forwarded only the `fields` third of what its loader knows, so a failed field catalog rendered as "No object bound".
✅ Console Performance Budget
📦 Bundle Size Report
Size Limits
|
os-support-ai
commented
Aug 20, 2026
ACCEPT — PM review, round 17.
The shape is the one the ruling wanted, and it has no escape hatch
One structural improvement beyond the card: The tightening found a live defect the card never named
This is the argument for doing it at the type level rather than adding a runtime check, made concrete. Two judgement calls I agree with
The type pin uses Verification, mixed direction, tool named per legLeg 1 (loosen One unpredicted extra red, reported rather than absorbed: PIN D also flipped to TS2578, because the already-failing sibling property in the same object literal suppresses the excess-property check PIN D relies on. Same direction, one more red — and the dev's own framing is right: "the template's prediction was incomplete rather than wrong." Leg 2 is the honest limit of the type half, and the reason both halves exist: a caller who hand-narrows around the accessor ( The card's count was corrected by measurement: 9 catalog reads in Merging via the queue. Generated by Claude Code |
Uh oh!
There was an error while loading. Please reload this page.
Fixes#5228
WidgetContextspelled a failed option catalog as an empty array, with the faultparked on a side channel and the requirement to consult it living in a doc
comment. This makes each catalog the four-arm
LoadStatethe loaders alreadyproduce, so the wrong read is a compile error at exactly the sites that must
decide what a failure looks like.
Reference implementation followed throughout: PR #5263 (objectui#5227), which
landed the shared
PickerLoadFailurerender and movedusePickerLoadintoloadState. No second failure presentation was invented.The residue, restated
Three loaders in
ResourceEditPagewere fixed by #5170 / #5169, but the fix wasprojected back down at the boundary: the catalogs stayed plain arrays,
[]on afailed load and byte-identical to a load that completed and found nothing, with
the fault travelling alongside on
catalogErrorsplus a*Loadingflag. Thecontract lived in
CatalogErrors' own doc comment, which had to open with "apicker MUST consult this before it renders its catalog". One line was enough to
ignore it:
Type-correct, natural to read, and it renders a refusal or an expired session as
the metadata graph's own answer of "this object has no fields".
The move
Each catalog is handed over intact as
LoadState< T >. Two pieces make thefailure decision a precondition rather than a convention:
FaultFreeLoadState< T >— every arm except the failure.LoadStateisdeclared in terms of it, so the two cannot drift.
offeredOptions(state, empty)— takesFaultFreeLoadState, so a caller stillholding the
errorarm cannot call it.const fields = offeredOptions(...)does not compile until the line above it has decided what a failure renders as.
That is the difference from
loadedData, which accepts any arm and can thereforeonly ask, in prose, for the same discipline. No
?? []convenience overloadwas left anywhere on the boundary, and this is a type change only — no runtime
check was added.
objectFieldsandobjectActionsare now derived from the ONE catalog state viamapLoaded, because they come from oneclient.get('object', …)request. Theyagree by construction instead of by the reader knowing they share a fetch — which
is what the old
catalogErrors.fieldskey, consulted by the action picker tolearn whether actions had failed, quietly asked of them.
Re-measured on today's
main(58398ba)The card priced 11 reachable sites + 1 and a fixture radius of ~17 lines / 5
files. Re-measured, since the card is ~24h old and this directory took PR5263 and
PR5327 since:
widgets.tsxSchemaForm.tsxdetectFieldRefWidgetinspectors/ViewVariantInspector.tsx— missed by the card, found by the compilerThe nine reads in
widgets.tsx:RefObjectWidget,ObjectSelectorWidget,FieldRefWidget,ViewRefWidget,FieldRefMultiWidget,FilterModeWidget,ActionMultiWidget,FilterBuilderWidget,ConditionWidget.SchemaForm.tsx's site is deliberately not migrated: it tests WIRING(
if (!widgetContext?.objectFields)), not contents. Every arm must answer yes,or a failed catalog would silently demote the picker back to the free-text input
whose typos it exists to prevent. Annotated in place rather than changed.
A live fault the tightening surfaced
inspectors/ViewVariantInspector.tsxis a second host ofWidgetContext, and thecard never named it. Its memo forwarded only
fieldsout of the tripleuseObjectFieldsreports (fields/loading/error), so a failed fieldcatalog reached its
field-ref/field-multipickers as[]and the pickersaid "No object bound" about an object that IS bound. That is the #5170 defect
class, still open on this host, and it is here precisely because nothing in the
old type required a producer to carry the fault at all — the array WAS the
contract. It cannot be dropped now; the three arms are spelled out. No
idlearmis synthesized (that would change what an unbound inspector renders, a separate
question).
Tests
New:
WidgetContext.catalogUnion.test.tsx— 21 tests, two halves that fail indifferent tools on purpose.
Compile-time (
tsc -p tsconfig.test.json, the only project that compiles thisdirectory's tests). Five
@ts-expect-errorpins, chosen over a type-assertionhelper because each is inherently two-way:
@ts-expect-erroris itself an error(TS2578) once the line below it starts compiling, so a loosening turns the file
red instead of quietly passing.
?? []read. Fails when the boundary is tightened;reports TS2578 when it is loosened back to an array. This is the card's pin.
offeredOptionsrefuses a state that still carries theerrorarm, with a positive control right below it (the same state, narrowed, is
accepted) so PIN B cannot pass merely by the accessor being uncallable.
catalogErrorsandobjectsLoadingare gone, not merely unused,so the old shape cannot creep back one key at a time.
idlearm (compiles; type-checks thesubstitution every migrated picker performs).
Runtime (vitest). For every migrated call site: a COMPLETED-but-empty catalog
and a FAILED catalog render differently, asserted in both directions, and the
failure arm renders the SHARED
PickerLoadFailureblock — read by its test id,its heading and its cause, so a bespoke second presentation fails here. The
empty arm is what stops the failure assertions being tautologies. Also pinned:
a failed FIELD catalog no longer posts a banner on the ACTION picker.
Two measurements worth recording.
filter-builderkeeps its failure block insidea Radix popover that does not open in jsdom, so the popover parts are rendered
inline via
vi.mock— this repo's existing way of reaching such content (theInboxPopoversuites). And the shared failure heading is not unique onscreen for the ordered-set pickers:
field-multiandaction-multialso putthat sentence in their add-trigger placeholder, so the helper asserts
withinthe block rather than document-wide.
Reverse verification — predicted before running, per leg, per tool
No build artifact sits between an edit and either subject: the pins import
./widgets/./loadStateas same-package relative source, and the root vitestconfig aliases every workspace name to
src. The artifact matters only for thecross-package imports these files also pull, which is why
pnpm --filter '@object-ui/app-shell^...' buildwas run first — without ittype-checkis afalse red (TS2307 on every workspace import). It was run; both
tscprojectsthen pass.
Leg 1 — loosen
WidgetContext.objectFieldsback to a plain array.tsc -p tsconfig.test.jsoncatalogUnion.test.tsx(107,3): error TS2578: Unused '@ts-expect-error' directive, 36 errors totaltsc --noEmit(source)objectFieldsconsumerswidgets.tsx(1248,19)etc.,ResourceEditPage.tsx(744,7),ViewVariantInspector.tsx(306,54)Not predicted: PIN D flipped to TS2578 as well (line 146), because the
already-failing sibling property in the same literal suppresses the excess-property
check that PIN D relies on. Same direction, one extra red.
Leg 2 — keep the union, hand-narrow
field-refso it drops the failure arm(
status === 'loaded' ? data : [], the early return deleted — what a caller whobypasses
offeredOptionscan still write).tscboth projectsfield-ref … a FAILED catalog replaces the picker with the shared failure blockLeg 2 is the honest limit of the type half and the reason both halves exist: the
compiler forces the decision at every site that uses the accessor, and vitest
catches the one who hand-narrows around it. Restore after each leg was
git checkoutfrom the commit, verified byte-identical (git statusandgit diff --statboth empty).Gates — union run on the final commit
79424e1bfpnpm --filter @object-ui/app-shell type-check(tsc --noEmit && tsc -p tsconfig.test.json) — exit 0, script name echoednpx vitest run packages/app-shell/src/views/metadata-adminfrom the repo root (CI's config, not the package-scoped one) — 186 files, 1941 passed, 1 skipped, 0 failedcheck-changeset-presence.mjs— 11 guarded source files, 1 changeset ·check-changeset-fixed.mjs·check-changeset-no-major.mjs(patch)check-control-bytes.mjs— OK, plus a manual control-byte scan of the changed filescheck-type-check-coverage.mjs— 45/46 and 41/41, unchangedcheck-i18n-call-site-keys.mjs— OK ·check-i18n-dead-keys.mjs— report only, no new hitseslint packages/app-shell— 0 errors (2493 pre-existing warnings, none added: the one new warning this branch introduced was fixed)Scope
Clause-② no: this tightens a type at a boundary. Nothing widens; the accepted set
of authored metadata does not move. Untouched, as instructed:
packages/components/src/renderers/complex/data-table.tsx(#5120),packages/plugin-grid/src/ObjectGrid.tsx(#5349),packages/plugin-dashboard/**.FieldSelectorWidgetis deliberately left as PR #5263 wrote it. It is not aWidgetContextconsumer — its state is component-local — and it already checksits error arm before rendering. Churning freshly-reviewed code for stylistic
consistency was not worth the review surface.
No docs change:
WidgetContextappears in no README or guide page (grepped);it is an internal type of the metadata-admin engine.
Serial disciplines for the app-shell subdirectory-level exemption, with #5216
(PR5354) open in
src/hooks/,src/urlParams.ts,src/views/ObjectView.tsx,src/console/home/,src/environment/: (a) surface declared to region level(
packages/app-shell/src/views/metadata-admin/**, disjoint from PR5354'ssubtrees); (b)
mainmerged once before opening this PR — already up to dateat 58398ba; (c)
mainto be merged again after PR5354 lands; (d) anyconflict goes to the merge queue, nothing hand-ordered.
Generated by Claude Code