fix(app-shell): render InspectorSelectField's placeholder, unreachable at all 45 call sites

.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 `<Select.Item value="">` 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.

packages/app-shell/src/views/metadata-admin/inspectors/FlowNodeInspector.declaredDefault.test.tsx

@@ -24,10 +24,13 @@
  *
  *  1. On a BOOLEAN control a declared default now seeds the value: an unset key
  *     draws the declared state, a stored value beats it, and nothing is written
- *     to the node. On a SELECT it still reaches nothing — an unset key draws a
- *     blank trigger, not the declared option and not even a placeholder,
- *     because the field always passes a controlled value and Radix renders its
- *     placeholder only for an undefined one (objectui#8450).
+ *     to the node. On a SELECT it still reaches nothing — an unset key draws
+ *     `InspectorSelectField`'s own em-dash PLACEHOLDER, which says "nothing is
+ *     selected" and is not anything the table declared. (objectui#8450 made
+ *     that placeholder reachable at all; before it the trigger was blank,
+ *     because 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. Both writers of the property feed the repaired boolean — the hand-written
  *     table here and the engine-published `configSchema` that
  *     `json-schema-to-fields` converts (`default: true` -> `defaultValue:
@@ -166,15 +169,16 @@ describe('select: a declared defaultValue still does not reach the control (obje
 
     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',
@@ -315,8 +319,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();
   });
 });

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 `<Select.Item value="">`
- * 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 `<Select.Item value="">` 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 `<SelectValue placeholder={placeholder} />`
+ * 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,21 +56,31 @@ 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 `<Label>`. */
+function trigger(name = 'Group'): HTMLElement {
+  const el = screen.queryByRole('combobox', { name });
+  expect(el, `a combobox named "${name}" is rendered at all`).not.toBeNull();
+  return el as HTMLElement;
+}
+
 describe('InspectorSelectField — empty-value option', () => {
   it('renders with an empty-string option without throwing', () => {
     expect(() =>
       render(
         <InspectorSelectField label="Group" value="" options={OPTIONS} onCommit={vi.fn()} />,
       ),
     ).not.toThrow();
-    expect(screen.getByText('Group')).toBeInTheDocument();
+    expect(screen.queryByText('Group'), 'the field label is rendered').not.toBeNull();
   });
 
   it('shows the selected non-empty option label on the trigger', () => {
     render(
       <InspectorSelectField label="Group" value="profile" options={OPTIONS} onCommit={vi.fn()} />,
     );
-    expect(screen.getByText('Profile')).toBeInTheDocument();
+    expect(screen.queryByText('Profile'), 'the selected option label reaches the DOM').not.toBeNull();
   });
 
   it('displays the empty-valued option label when value is "" (round-trips through the sentinel)', () => {
@@ -47,10 +93,140 @@ describe('InspectorSelectField — empty-value option', () => {
         placeholder="Pick one"
       />,
     );
-    // value "" matches the "— No group —" option (both bridged to the
-    // sentinel), so the trigger surfaces that label rather than the
-    // placeholder — confirming the "" ⇄ none round-trip works.
-    expect(screen.getByText('— No group —')).toBeInTheDocument();
-    expect(screen.queryByText('Pick one')).not.toBeInTheDocument();
+    // ⚠️ The reason this case originally gave for the placeholder's absence was
+    // NOT the operative one. It read "value '' matches the — No group — option,
+    // so the trigger surfaces that label rather than the placeholder" — true of
+    // the first assertion, but the second one held for a different reason
+    // entirely: before objectui#8450 the placeholder was unreachable in EVERY
+    // state, matching option or not, so this case could not have told a working
+    // field from a broken one. It can now: with the roster below stripped of
+    // its "" row the very same props render "Pick one" (next describe), so this
+    // absence is the "" option winning, which is what it always claimed to be.
+    expect(
+      screen.queryByText('— No group —'),
+      'the "" option is a real selection and its label wins',
+    ).not.toBeNull();
+    expect(
+      screen.queryByText('Pick one'),
+      'a selection is showing, so the placeholder stays out of the DOM',
+    ).toBeNull();
+    expect(
+      trigger().textContent,
+      'the trigger shows the option label ALONE — not the label plus the placeholder',
+    ).toBe('— No group —');
+  });
+});
+
+describe('InspectorSelectField — placeholder (objectui#8450)', () => {
+  it('renders the declared "—" default when the value is undefined', () => {
+    render(
+      <InspectorSelectField label="Group" value={undefined} options={NO_NONE_OPTIONS} onCommit={vi.fn()} />,
+    );
+    expect(
+      trigger().textContent,
+      'an unset select draws its placeholder, not a blank trigger',
+    ).toBe('—');
+  });
+
+  it('renders the declared "—" default for an empty-string value', () => {
+    render(
+      <InspectorSelectField label="Group" value="" options={NO_NONE_OPTIONS} onCommit={vi.fn()} />,
+    );
+    expect(
+      trigger().textContent,
+      '"" with no "" option means the same thing as undefined: nothing is selected',
+    ).toBe('—');
+  });
+
+  it("renders the CALLER's placeholder — the prop that reached no output before", () => {
+    render(
+      <InspectorSelectField
+        label="Group"
+        value=""
+        options={NO_NONE_OPTIONS}
+        onCommit={vi.fn()}
+        placeholder="Pick one"
+      />,
+    );
+    expect(
+      screen.queryByText('Pick one'),
+      "the caller's placeholder reaches the DOM",
+    ).not.toBeNull();
+    expect(trigger().textContent, 'and it is the whole of what the trigger shows').toBe('Pick one');
+  });
+
+  it('does NOT render the placeholder when a value is selected', () => {
+    render(
+      <InspectorSelectField
+        label="Group"
+        value="profile"
+        options={NO_NONE_OPTIONS}
+        onCommit={vi.fn()}
+        placeholder="Pick one"
+      />,
+    );
+    // The half that fails on a field which renders the placeholder ALWAYS —
+    // an implementation strictly worse than the bug it replaces.
+    expect(screen.queryByText('Pick one'), 'the placeholder is not in the DOM').toBeNull();
+    expect(
+      trigger().textContent,
+      'the trigger shows the selected label and nothing else',
+    ).toBe('Profile');
+  });
+
+  it('does NOT render the placeholder when "" is itself an offered option', () => {
+    render(
+      <InspectorSelectField
+        label="Group"
+        value=""
+        options={OPTIONS}
+        onCommit={vi.fn()}
+        placeholder="Pick one"
+      />,
+    );
+    expect(screen.queryByText('Pick one'), 'the "none" row is the selection').toBeNull();
+    expect(trigger().textContent, 'so its label is what shows').toBe('— No group —');
+  });
+
+  it('flips both ways as the value comes and goes', () => {
+    const { rerender } = render(
+      <InspectorSelectField label="Group" value={undefined} options={NO_NONE_OPTIONS} onCommit={vi.fn()} />,
+    );
+    expect(trigger().textContent, 'empty at first').toBe('—');
+
+    rerender(
+      <InspectorSelectField label="Group" value="meta" options={NO_NONE_OPTIONS} onCommit={vi.fn()} />,
+    );
+    expect(
+      trigger().textContent,
+      'a value arriving replaces the placeholder with the option label — the Radix ' +
+        'item text still portals into SelectValue across the swap',
+    ).toBe('Metadata');
+
+    rerender(
+      <InspectorSelectField label="Group" value="" options={NO_NONE_OPTIONS} onCommit={vi.fn()} />,
+    );
+    expect(trigger().textContent, 'and clearing it brings the placeholder back').toBe('—');
+  });
+
+  it('carries Radix\'s own data-placeholder flag in the empty state only', () => {
+    // Not decoration: `data-[placeholder]:text-muted-foreground` on the Shadcn
+    // trigger is how the empty state is greyed, and Radix cannot derive the
+    // attribute itself while the sentinel keeps its value non-empty.
+    const { rerender } = render(
+      <InspectorSelectField label="Group" value="" options={NO_NONE_OPTIONS} onCommit={vi.fn()} />,
+    );
+    expect(
+      trigger().getAttribute('data-placeholder'),
+      'the empty trigger is flagged as showing a placeholder',
+    ).toBe('');
+
+    rerender(
+      <InspectorSelectField label="Group" value="profile" options={NO_NONE_OPTIONS} onCommit={vi.fn()} />,
+    );
+    expect(
+      trigger().getAttribute('data-placeholder'),
+      'a selected trigger is not',
+    ).toBeNull();
   });
 });

packages/app-shell/src/views/metadata-admin/inspectors/_shared.tsx

@@ -253,6 +253,31 @@ export function InspectorSelectField({
   // renders the real `button[role=combobox]`, which is a labelable element, so
   // one `for`/`id` pair names it (no second `aria-labelledby` channel needed).
   const id = React.useId();
+  // `SelectValue`'s own `placeholder` is unreachable here, and was at all 45
+  // call sites (objectui#8450). Radix shows it only when its value is `''` or
+  // `undefined`, and the sentinel bridge above guarantees the value is never
+  // either — a controlled value matching no `SelectItem` renders as nothing at
+  // all, so every empty select in the designer drew a BLANK trigger.
+  //
+  // The repair renders the placeholder here instead of handing Radix an
+  // `undefined` value. Passing `undefined` would work, but it makes the Radix
+  // `Select` UNCONTROLLED for exactly as long as the field is empty, so the
+  // first selection at every call site flips it back — measured: Radix logs
+  // `Select is changing from uncontrolled to controlled`, and the same flip
+  // fires with the value untouched when a late-arriving `options` list gains or
+  // loses its `''` row (async pickers: `useMetaOptions`, `datasetOptions`,
+  // `fieldOptions`). While uncontrolled, Radix also keeps its OWN value, so a
+  // pick the owner declines to persist stays on screen. Rendering the text is
+  // the same predicate with none of that.
+  //
+  // "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.
+  const hasNoneOption = options.some((o) => o.value === '');
+  const showPlaceholder = (value ?? '') === '' && !hasNoneOption;
   return (
     <div className="space-y-1">
       <Label htmlFor={id} className="text-xs text-muted-foreground">{label}</Label>
@@ -261,8 +286,17 @@ export function InspectorSelectField({
         onValueChange={(v) => onCommit(fromInner(v))}
         disabled={disabled}
       >
-        <SelectTrigger id={id} className="h-8 text-sm">
-          <SelectValue placeholder={placeholder} />
+        {/* `data-placeholder` is Radix's own trigger flag and the hook Shadcn's
+            `data-[placeholder]:text-muted-foreground` styles the empty state
+            with. Radix cannot derive it through the sentinel, and the trigger
+            spreads caller props AFTER its own attributes, so setting it here
+            restores the real placeholder styling instead of hand-rolling it. */}
+        <SelectTrigger
+          id={id}
+          className="h-8 text-sm"
+          data-placeholder={showPlaceholder ? '' : undefined}
+        >
+          {showPlaceholder ? <span>{placeholder}</span> : <SelectValue />}
         </SelectTrigger>
         <SelectContent>
           {options.map((o) => (