diff --git a/.changeset/8488-inspector-select-unknown-value.md b/.changeset/8488-inspector-select-unknown-value.md new file mode 100644 index 0000000000..ffab1e137f --- /dev/null +++ b/.changeset/8488-inspector-select-unknown-value.md @@ -0,0 +1,34 @@ +--- +'@object-ui/app-shell': patch +--- + +`InspectorSelectField` surfaces a stored value the option roster does not offer, +instead of painting a blank trigger (objectui#8488). + +Radix renders a controlled value matching no `SelectItem` as nothing at all, so a +metadata key holding a stale or off-spec value drew an EMPTY select in the designer — +pixel-identical to "this field is unset". The author saw an empty control, picked a +value to fill it in, and silently overwrote a key they were never shown. + +The field now synthesises a selectable, flagged row for a non-empty value the roster +does not contain, labelled `VALUE (not found)` by default. Every call site inherits it. + +Deliberately NOT the placeholder: drawing `—` over an unknown value would assert +"nothing is stored" about a field that is storing something — confidently wrong rather +than merely blank. objectui#8450 keeps owning the empty state; the two stay +distinguishable on screen, which is pinned as its own case. + +Eight call sites had hand-rolled the rule and are now deleted in favour of it. Three +had flagged their row — `ActionTargetField` (`(not found)`), the view-column field +picker (`(not in object)`) and `FlowNodeConfigField`'s select branch (`(deprecated)`, +framework#4278 / ADR-0090 D3). Five had made the value visible but UNFLAGGED — +`ReportDefaultInspector`'s type, dataset and both chart axes, and +`ViewVariantInspector`'s type — so a value the catalog had dropped read exactly like +one it offered. Those five now flag it. Wording stays with the call site through the +new optional `unknownValueLabel` prop, so `(deprecated)` still says the thing +`(not found)` does not. + +Known boundary, unchanged by this: an async picker that has not loaded yet is +indistinguishable from an empty roster, so a valid value briefly wears the flag while +its options are in flight. That blind spot is inherited from the hand-rolled copies, +not introduced here, and is tracked separately. diff --git a/packages/app-shell/src/views/metadata-admin/inspectors/ActionDefaultInspector.tsx b/packages/app-shell/src/views/metadata-admin/inspectors/ActionDefaultInspector.tsx index b403686769..37c121c1b2 100644 --- a/packages/app-shell/src/views/metadata-admin/inspectors/ActionDefaultInspector.tsx +++ b/packages/app-shell/src/views/metadata-admin/inspectors/ActionDefaultInspector.tsx @@ -202,7 +202,10 @@ const TARGET_META_TYPE: Record = { flow: 'flow', modal: 'page', /** Target binding: a reference picker for flow/modal/form (names come from the * matching metadata type), else a free-text input (url/api). An out-of-catalog - * value is preserved as a synthesized option so it is never silently dropped. */ + * value is preserved as a synthesized option so it is never silently dropped — + * `InspectorSelectField` owns that rule now (objectui#8488); this call site + * used to hand-roll it, in the same `VALUE (not found)` wording the primitive + * defaults to, so deleting the copy changes no text. */ function ActionTargetField({ type, value, onCommit, cfg, readOnly }: { type: string; value: string; @@ -213,13 +216,10 @@ function ActionTargetField({ type, value, onCommit, cfg, readOnly }: { const metaType = TARGET_META_TYPE[type] ?? null; const { options } = useMetaOptions(metaType); const usePicker = !!metaType && options.length > 0; - const opts = usePicker && value && !options.some((o) => o.value === value) - ? [{ value, label: `${value} (not found)` }, ...options] - : options; return (
{usePicker ? ( - + ) : ( )} diff --git a/packages/app-shell/src/views/metadata-admin/inspectors/FlowNodeConfigField.tsx b/packages/app-shell/src/views/metadata-admin/inspectors/FlowNodeConfigField.tsx index 444929c8ff..4f0e290195 100644 --- a/packages/app-shell/src/views/metadata-admin/inspectors/FlowNodeConfigField.tsx +++ b/packages/app-shell/src/views/metadata-admin/inspectors/FlowNodeConfigField.tsx @@ -258,15 +258,19 @@ export function FlowNodeConfigField({ field, value, onCommit, disabled, locale, // render, or editing a legacy node would silently blank it. Surface // it as selectable but flag it — it is not offered to fresh nodes. // Same rule as FlowObjectListField's select cells (ADR-0090 D3). - const shown = - current && !opts.some((o) => o.value === current) - ? [...opts, { value: current, label: `${current} (deprecated)` }] - : opts; + // + // objectui#8488 promoted that RULE into `InspectorSelectField`, where + // the other 42 call sites inherit it. What stays here is the WORDING: + // "(deprecated)" is sourced (framework#4278, ADR-0090 D3) and says + // something the generic "(not found)" does not — the value is known, + // it is simply no longer offered. The prop exists so this sentence + // survives the unification instead of being flattened by it. return ( `${v} (deprecated)`} onCommit={(v) => onCommit(v)} disabled={disabled} /> diff --git a/packages/app-shell/src/views/metadata-admin/inspectors/ReportDefaultInspector.test.tsx b/packages/app-shell/src/views/metadata-admin/inspectors/ReportDefaultInspector.test.tsx index 897d9f64aa..76b6ab5c3d 100644 --- a/packages/app-shell/src/views/metadata-admin/inspectors/ReportDefaultInspector.test.tsx +++ b/packages/app-shell/src/views/metadata-admin/inspectors/ReportDefaultInspector.test.tsx @@ -7,6 +7,18 @@ import type { DatasetCatalogEntry } from '../previews/useDatasetCatalog'; afterEach(cleanup); +// One fetch double at MODULE scope, never torn down (objectui#7439 / #6640). +// `useDatasetSemantics` hydrates a bound dataset the catalog does not carry +// with `client.get('dataset', name)`, and happy-dom resolves that relative URL +// to a live socket on localhost:3000. The read is fire-and-forget — no barrier +// in the test body awaits it — so an `afterEach` teardown would restore the +// real fetch while the tree is still mounted. Installing it here means no test +// in this file can ever end with the real fetch back in place. +vi.stubGlobal( + 'fetch', + vi.fn(async () => new Response('null', { status: 404, headers: { 'content-type': 'application/json' } })), +); + // ADR-0021 single-form: the catalog override short-circuits the dataset // fetches so the inspector renders with zero context / transport dependency. const catalog: DatasetCatalogEntry[] = [ @@ -194,3 +206,48 @@ describe('ReportDefaultInspector — dataset binding (9.0 single form)', () => { expect(labelledInput('Label')).toBeDisabled(); }); }); + +/** + * objectui#8488 — one of the 42 call sites that never hand-rolled the rule. + * + * `datasetName` is offered against `datasetOptions`, so a dataset dropped from + * the catalog used to leave the binding BLANK — a report that reads as "no + * dataset bound" while `dataset: 'retired_dataset'` sits in the draft. This + * inspector wrote no repair of its own; it inherits `InspectorSelectField`'s. + * + * The pin reads the trigger's text, not `datasetOptions`: the bug's whole shape + * was a correct `value` prop that reached no pixels. + */ +describe('ReportDefaultInspector — a dataset the catalog no longer lists (objectui#8488)', () => { + const datasetTrigger = () => screen.getByRole('combobox', { name: 'Dataset' }); + + it('shows the stored dataset name, flagged, instead of a blank binding', () => { + render( + , + ); + expect( + datasetTrigger().textContent, + 'the binding names what it is bound to, even though the catalog dropped it', + ).toBe('retired_dataset (not found)'); + }); + + it('still shows a catalogued dataset by its own label — the flag is not unconditional', () => { + render( + , + ); + // `datasetOptions` labels a catalogued dataset `LABEL (NAME)`; the point of + // the case is the absence of the flag, not the label's own spelling. + expect(datasetTrigger().textContent).toBe('Sales metrics (sales_metrics)'); + expect(screen.queryByText(/not found/), 'nothing is flagged').toBeNull(); + }); +}); diff --git a/packages/app-shell/src/views/metadata-admin/inspectors/ReportDefaultInspector.tsx b/packages/app-shell/src/views/metadata-admin/inspectors/ReportDefaultInspector.tsx index 3faab04e78..fc9f0c03a2 100644 --- a/packages/app-shell/src/views/metadata-admin/inspectors/ReportDefaultInspector.tsx +++ b/packages/app-shell/src/views/metadata-admin/inspectors/ReportDefaultInspector.tsx @@ -95,7 +95,7 @@ const TYPE_LABEL_KEYS: Record = { }; /** Build the Report-type options from the spec `type` enum. */ -function useTypeOptions(currentType: string, locale?: string) { +function useTypeOptions(locale?: string) { return React.useMemo(() => { const schema = getListVariantSchema(); const rawEnum = schema?.properties?.type?.enum; @@ -190,11 +190,13 @@ function useTypeOptions(currentType: string, locale?: string) { ? rawEnum.filter((v: unknown): v is string => typeof v === 'string') : ['grid', 'kanban', 'calendar', 'gallery', 'gantt', 'timeline']; const opts = values.map((v) => ({ value: v, label: typeLabel(v, locale) })); - if (!opts.some((o) => o.value === currentType) && currentType) { - opts.push({ value: currentType, label: typeLabel(currentType, locale) }); - } + // objectui#8488 — a `type` the spec enum does not carry used to be appended + // here UNFLAGGED, so an off-spec view type read exactly like an offered + // one. `InspectorSelectField` synthesises and flags it now; `typeLabel` + // still gets its say through `unknownValueLabel` at the call site, since a + // retired type can still have a human name in `TYPE_LABELS`. return opts; - }, [currentType, locale]); + }, [locale]); } export function ViewVariantInspector({ @@ -216,7 +218,7 @@ export function ViewVariantInspector({ const isFormFamily = isFormFamilyKey(familyKey ?? variantKey); const viewType = typeof variant.type === 'string' ? (variant.type as string) : 'grid'; - const typeOptions = useTypeOptions(viewType, locale); + const typeOptions = useTypeOptions(locale); const binding = readObjectBinding(variant, draft); // Canonical label lives at the top level (`draft.label`); a list `config` @@ -394,6 +396,7 @@ export function ViewVariantInspector({ label={t('engine.inspector.view.type', locale)} value={viewType} options={typeOptions} + unknownValueLabel={(v) => `${typeLabel(v, locale)} (not found)`} onCommit={(v) => writeVariant({ type: v })} disabled={readOnly} /> diff --git a/packages/app-shell/src/views/metadata-admin/inspectors/_shared.tsx b/packages/app-shell/src/views/metadata-admin/inspectors/_shared.tsx index ccdf0f8872..e694b0bd8d 100644 --- a/packages/app-shell/src/views/metadata-admin/inspectors/_shared.tsx +++ b/packages/app-shell/src/views/metadata-admin/inspectors/_shared.tsx @@ -225,12 +225,22 @@ export function InspectorNumberField({ ); } +/** + * Default wording for the row {@link InspectorSelectField} synthesises when the + * stored value is not in the roster. Raw English, like `placeholder`'s `'—'` + * default: this module takes raw strings and has no locale in scope (see the + * file header). Call sites with a better word for their own domain pass + * `unknownValueLabel` — that is the prop's whole reason to exist. + */ +const defaultUnknownValueLabel = (v: string) => `${v} (not found)`; + export function InspectorSelectField({ label, value, options, onCommit, placeholder = '—', + unknownValueLabel = defaultUnknownValueLabel, disabled, }: { label: string; @@ -238,6 +248,12 @@ export function InspectorSelectField({ options: Array<{ value: string; label: string }>; onCommit: (v: string) => void; placeholder?: string; + /** + * Wording for the synthesised row that carries a stored value the roster does + * not offer. Receives the raw stored value; defaults to `VALUE (not found)`. + * Override it, never the RULE — the rule is the one this primitive owns. + */ + unknownValueLabel?: (value: string) => string; disabled?: boolean; }) { // Radix `` forbids an empty-string value (it reserves "" @@ -273,11 +289,50 @@ export function InspectorSelectField({ // "No selection" is the narrow state: no value AND no option standing for // none. When the caller DOES offer a `''` row (a "— None —" choice), `''` is // a selection like any other and that row's label wins — the case pinned in - // `_shared.select.test.tsx`. A non-empty value matching no option keeps - // rendering blank, unchanged: that is a stale/unknown value, not an empty - // one, and the call sites that care already synthesise a visible row for it. + // `_shared.select.test.tsx`. + const current = value ?? ''; const hasNoneOption = options.some((o) => o.value === ''); - const showPlaceholder = (value ?? '') === '' && !hasNoneOption; + const showPlaceholder = current === '' && !hasNoneOption; + + // objectui#8488 — the OTHER half, and it is not the same state. A non-empty + // value the roster does not offer is a STALE value, not an absent one, and + // Radix renders a controlled value matching no `SelectItem` as nothing at + // all: the trigger goes blank, pixel-identical to "unset". The author then + // sees an empty control, picks something to "fill it in", and overwrites a + // key they were never shown. + // + // ⛔ The placeholder is NOT the repair. Drawing `'—'` here would assert + // "nothing is stored" about a field that IS storing something — worse than + // the blank, because it is confidently wrong rather than merely empty. So + // the two states must stay distinguishable on screen, and they are: empty + // draws `placeholder`, unknown draws the value itself under a flag. + // + // EIGHT call sites had hand-rolled this, and re-deriving them is what moved + // the rule here. Three flagged the row — `ActionTargetField` + // ("(not found)"), `ViewColumnInspector`'s field picker ("(not in object)") + // and `FlowNodeConfigField`'s select branch ("(deprecated)", framework#4278 / + // ADR-0090 D3). FIVE only made it visible — `ReportDefaultInspector`'s type, + // dataset and both chart axes, and `ViewVariantInspector`'s type — appending + // the raw value with no marker at all, which is the same screen a real option + // draws. Every one of them is deleted now; only the WORDING stays with the + // call site, through `unknownValueLabel`. + // + // The row is selectable — all three flagged copies made it so, and re-picking + // your own stored value must not be a dead end. It goes FIRST, where two of + // those three put it: Radix scrolls the selected item into view on open, so + // the real roster then sits directly under it, which is where a replacement + // gets picked. + // + // ⚠️ Measured boundary, deliberately not repaired here: this cannot tell "the + // roster is empty" from "the roster has not loaded yet", so a valid value + // wears the flag for as long as an async picker is still fetching. That blind + // spot is inherited, not introduced — `ViewColumnInspector`'s hand-rolled + // copy had it too — and closing it needs a loading contract this primitive + // does not have. Filed separately. + const isUnknownValue = current !== '' && !options.some((o) => o.value === current); + const shownOptions = isUnknownValue + ? [{ value: current, label: unknownValueLabel(current) }, ...options] + : options; return (
@@ -299,7 +354,7 @@ export function InspectorSelectField({ {showPlaceholder ? {placeholder} : } - {options.map((o) => ( + {shownOptions.map((o) => ( {o.label} ))} diff --git a/packages/app-shell/src/views/metadata-admin/inspectors/_shared.unknownValue.test.tsx b/packages/app-shell/src/views/metadata-admin/inspectors/_shared.unknownValue.test.tsx new file mode 100644 index 0000000000..f03f0c47db --- /dev/null +++ b/packages/app-shell/src/views/metadata-admin/inspectors/_shared.unknownValue.test.tsx @@ -0,0 +1,170 @@ +// Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * objectui#8488 — what `InspectorSelectField`'s trigger renders for a stored + * value the option roster does NOT offer. + * + * Radix renders a controlled value matching no `SelectItem` as nothing at all, + * so such a value used to paint a BLANK trigger — pixel-identical to "this + * field is unset". The author sees an empty control, picks a value to fill it + * in, and silently overwrites a key they were never shown. + * + * Eight call sites had hand-rolled it. THREE flagged the synthesised row, in + * three wordings: `ActionTargetField` ("(not found)"), `ViewColumnInspector`'s + * field picker ("(not in object)") and `FlowNodeConfigField`'s select branch + * ("(deprecated)", framework#4278 / ADR-0090 D3). The other FIVE — + * `ReportDefaultInspector`'s type, dataset and both chart axes, and + * `ViewVariantInspector`'s type — appended the raw value with NO marker, which + * is the refused direction: it makes "stored" and "offered" the same picture. + * The rule is the primitive's now; only the wording stayed with the call site, + * through `unknownValueLabel`. + * + * ## What these cases assert, and why it is the trigger text + * + * The pin reads the RENDERED TRIGGER, never `options.length` or the synthesised + * entry's presence in some array. An implementation that appends the row but + * fails to surface it — the exact shape of the original bug, since Radix's + * blank trigger coexisted with a perfectly good `value` prop — passes an + * options-array assertion and fails these. + * + * ## ⛔ The placeholder is not the repair (the "empty ≠ unknown" pin) + * + * `unknown value renders differently from an empty one` is the case that fails + * on the tempting wrong fix: papering the unknown value over with `placeholder` + * would assert "nothing is stored" about a field that IS storing something — + * confidently wrong rather than merely blank. objectui#8450 owns the empty + * half (empty draws the placeholder) and it must keep owning it, so the two + * states are read side by side here rather than one at a time. + * + * ## Boundary, measured and deliberately NOT repaired here + * + * The primitive cannot tell "roster is empty" from "roster has not loaded yet", + * so a valid value briefly wears the flag while an async picker + * (`useMetaOptions`, `useObjectFields`, `datasetOptions`) is still fetching. + * That is inherited behaviour, not new: `ViewColumnInspector`'s hand-rolled + * copy had exactly the same blind spot before this change. Filed separately. + */ + +import { describe, it, expect, vi, afterEach } from 'vitest'; +import { render, screen, cleanup } from '@testing-library/react'; +import { InspectorSelectField } from './_shared'; + +afterEach(cleanup); + +const OPTIONS = [ + { value: 'profile', label: 'Profile' }, + { value: 'meta', label: 'Metadata' }, +]; + +/** The trigger Radix renders — `button[role=combobox]`, named by the `