From fbd04bf8ce9efd99b8920e9acc6dcb7a05794eaa Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 8 Sep 2026 02:26:48 +0000 Subject: [PATCH] fix(app-shell): render InspectorSelectField's placeholder, unreachable at all 45 call sites The field bridges a caller's "" through an internal sentinel so a "- None -" option can exist at all (Radix `` throws on render). That bridge also guaranteed the value handed to Radix was never "" or undefined - the only two values for which Radix renders `SelectValue`'s placeholder - and a controlled value matching no `SelectItem` renders as nothing. So the declared "-" default reached no rendered output anywhere in the metadata-admin designer, and every empty select drew a blank trigger. Measured on the tree, not assumed: 45 non-test call sites, 0 of which pass a placeholder of their own, so what never rendered was the primitive's own default at every one of them. The trigger now renders the placeholder itself, on the narrow state that means "nothing is selected" - no value AND no option standing for none. Where the caller does offer a "" row, that row is a selection and its label still wins. `data-placeholder` is set alongside it so Shadcn's `data-[placeholder]:text-muted-foreground` still greys the empty state; Radix cannot derive that attribute through the sentinel, and its trigger spreads caller props after its own. Handing Radix an `undefined` value instead was measured and rejected: it makes the `Select` uncontrolled for exactly as long as the field is empty, so Radix logs `Select is changing from uncontrolled to controlled` on the first selection at every call site - and the same flip fires with the value untouched when a late-arriving `options` list gains or loses its "" row, which the async pickers (useMetaOptions, datasetOptions, fieldOptions) do routinely. While uncontrolled, Radix also keeps its own value, so a pick the owner declines to persist stays on screen. `_shared.select.test.tsx` case 3 kept its assertions and had its stated reason corrected: it read the placeholder's absence as the "" option winning, but before this change the placeholder was absent in every state, so the case could not have told a working field from a broken one. Its two "does not render" rows are now the non-regression half - they are what fails on a field that renders the placeholder always, an implementation strictly worse than the bug. `FlowNodeInspector.declaredDefault.test.tsx` reads '-' where it read '' on two rows. Its subject - that a declared `defaultValue` seeds no control - is unchanged; only the mark for "nothing is selected" moved from blank to the placeholder. Refs: objectui#8450 Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01YBWFb5YgMU5dw8p2VKj16S --- .../8450-inspector-select-placeholder.md | 27 +++ ...FlowNodeInspector.declaredDefault.test.tsx | 31 +-- .../inspectors/_shared.select.test.tsx | 200 ++++++++++++++++-- .../metadata-admin/inspectors/_shared.tsx | 38 +++- 4 files changed, 269 insertions(+), 27 deletions(-) create mode 100644 .changeset/8450-inspector-select-placeholder.md diff --git a/.changeset/8450-inspector-select-placeholder.md b/.changeset/8450-inspector-select-placeholder.md new file mode 100644 index 000000000..415678f5e --- /dev/null +++ b/.changeset/8450-inspector-select-placeholder.md @@ -0,0 +1,27 @@ +--- +'@object-ui/app-shell': patch +--- + +Make `InspectorSelectField`'s `placeholder` reach the rendered trigger +(objectui#8450). Every empty select in the metadata-admin designer drew a BLANK +trigger instead of its hint. + +The field bridges a caller's `''` through an internal sentinel so a "— None —" +option can exist at all (Radix `` throws). That bridge also +guaranteed the value handed to Radix was never `''` or `undefined` — the only two +values for which Radix renders `SelectValue`'s placeholder — and a controlled +value matching no `SelectItem` renders as nothing. So the declared `'—'` default +was unreachable at all 45 call sites, none of which passes a placeholder of its +own. + +The trigger now renders the placeholder itself, on the narrow state that means +"nothing is selected": no value AND no option standing for none. Where the caller +DOES offer a `''` row, that row is a selection and its label still wins, +unchanged. A non-empty value matching no option also still renders blank — +that is a stale value, not an empty one, and is out of this change's scope. + +Measured effect on the designer: 13 of the 45 call sites can be empty without +offering a "none" row, and those now show `'—'` where they showed nothing — the +flow-node config selects, the app-nav item type, the curated page-block props, +the report dataset/chart-axis pickers and the action target/variant/mode/component +pickers. The other 32 render exactly as before. diff --git a/packages/app-shell/src/views/metadata-admin/inspectors/FlowNodeInspector.declaredDefault.test.tsx b/packages/app-shell/src/views/metadata-admin/inspectors/FlowNodeInspector.declaredDefault.test.tsx index 0dd3dae7f..82e6b1bed 100644 --- a/packages/app-shell/src/views/metadata-admin/inspectors/FlowNodeInspector.declaredDefault.test.tsx +++ b/packages/app-shell/src/views/metadata-admin/inspectors/FlowNodeInspector.declaredDefault.test.tsx @@ -15,10 +15,13 @@ * The measurement, in four parts: * * 1. A declared default does NOT seed the control's value. An unset key draws - * an unchecked box / a blank select, whatever the table declares. The - * select is blank rather than showing a placeholder, because the field - * always passes a controlled value and Radix renders its placeholder only - * for an undefined one. + * an unchecked box / a select on its PLACEHOLDER, whatever the table + * declares. The placeholder is `InspectorSelectField`'s own em-dash, not + * anything the table said — it means "nothing is selected", which is + * precisely the point: the declared `'GET'` is nowhere on screen. (Until + * objectui#8450 the trigger was blank instead: the sentinel the field + * bridges `''` through kept Radix from ever recognising the empty state. + * Blank and em-dash are the same fact about `defaultValue`, told twice.) * 2. That holds on BOTH writers of the property — the hand-written table here * and the engine-published `configSchema` that `json-schema-to-fields` * converts (`default: true` -> `defaultValue: 'true'`). Same dead end. @@ -151,15 +154,16 @@ describe('a declared defaultValue does not reach the rendered control', () => { renderInspector(draftWith('http_request', { config: {} })); - // Measured, not assumed: the trigger is EMPTY — it shows neither the - // declared default nor `InspectorSelectField`'s own em-dash placeholder. - // Radix renders a placeholder only for an UNCONTROLLED/undefined value; the - // field always passes a controlled one (the `''` -> sentinel bridge), and a - // controlled value matching no `SelectItem` renders as nothing at all. + // Measured, not assumed: the trigger shows `InspectorSelectField`'s own + // em-dash placeholder — the "nothing is selected" mark — and NOT the + // declared default. (It rendered blank before objectui#8450, which fixed + // the placeholder's own unreachability in the shared primitive. That was a + // defect in the primitive, not evidence about `defaultValue`; this card's + // subject is the assertion below, which is unchanged either way.) expect( triggerText('Method'), - 'the Method trigger renders empty — no declared default, not even the placeholder', - ).toBe(''); + 'the Method trigger renders its placeholder — the declared default seeds nothing', + ).toBe('—'); expect( screen.queryByText('GET'), 'the declared default "GET" reaches no part of the rendered inspector', @@ -216,8 +220,9 @@ describe('the online writer of defaultValue hits the same dead end', () => { expect( triggerText('Method'), - 'the server-derived default is not seeded into the control either', - ).toBe(''); + 'the server-derived default is not seeded into the control either — the ' + + 'trigger sits on its placeholder, same as the hand-written half', + ).toBe('—'); expect(screen.queryByText('POST'), 'the derived default reaches no rendered node').toBeNull(); }); }); diff --git a/packages/app-shell/src/views/metadata-admin/inspectors/_shared.select.test.tsx b/packages/app-shell/src/views/metadata-admin/inspectors/_shared.select.test.tsx index 1ddaf35d4..c0762f85f 100644 --- a/packages/app-shell/src/views/metadata-admin/inspectors/_shared.select.test.tsx +++ b/packages/app-shell/src/views/metadata-admin/inspectors/_shared.select.test.tsx @@ -1,11 +1,47 @@ // Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license. /** - * Regression: InspectorSelectField must accept an option whose value is - * the empty string (a "— None —" choice). Radix `` - * throws on render; the field bridges "" through an internal sentinel. - * This is exactly what the object field "Group" selector relies on once - * field groups exist, so a regression here crashes the whole inspector. + * Two contracts of `InspectorSelectField`'s trigger, pinned together because + * they are the same question asked twice: what does the trigger render when the + * caller's value is EMPTY? + * + * 1. Regression: the field must accept an option whose value is the empty + * string (a "— None —" choice). Radix `` throws on + * render; the field bridges "" through an internal sentinel. This is exactly + * what the object field "Group" selector relies on once field groups exist, + * so a regression here crashes the whole inspector. + * + * 2. objectui#8450: when the caller offers NO such option, the empty state must + * draw the `placeholder`. It used not to. The sentinel that makes (1) work + * is also what broke (2): Radix renders `SelectValue`'s placeholder only for + * a value of `''` or `undefined` (`shouldShowPlaceholder`), the bridge + * guarantees the value is neither, and a controlled value matching no + * `SelectItem` renders as nothing at all. Measured across the tree: 45 + * non-test call sites, 0 of which passed an explicit `placeholder` — so what + * never rendered was the declared `'—'` default, at every empty select in + * the designer. + * + * ⚠️ The two pull against each other, which is why they live in one file. A + * repair that shows the placeholder whenever the value is empty breaks (1): the + * "— No group —" row IS a selection and its label must win. A repair that keeps + * quiet whenever the value is empty is the bug in (2). The predicate that + * satisfies both is narrow — no value AND no option standing for "none". + * + * ## The non-regression half (objectui#8350's lesson) + * + * Every positive case below is also satisfied by a field that renders the + * placeholder ALWAYS — an implementation strictly worse than the bug, since it + * would hide the selected value too. The two "does not render" cases are what + * fail on it, and they assert the trigger's exact text rather than the + * placeholder's absence, so an implementation that renders both also fails. + * + * ## Reverse verification + * + * Ablating the READ SITE — restoring `` + * inside an unconditional trigger, i.e. the pre-fix source — turns the three + * `renders …` rows red and leaves the two `does not render …` rows green. That + * asymmetry is the point: the pin fails for the bug's own reason and for no + * other. Run recorded on the PR. */ import { describe, it, expect, vi, afterEach } from 'vitest'; @@ -20,6 +56,16 @@ const OPTIONS = [ { value: 'meta', label: 'Metadata' }, ]; +/** The same roster minus the "none" row — the shape 35 of the 45 call sites use. */ +const NO_NONE_OPTIONS = OPTIONS.filter((o) => o.value !== ''); + +/** The trigger Radix renders — `button[role=combobox]`, named by the `