fix(app-shell): render InspectorSelectField's placeholder, unreachable at all 45 call sites - #8494
Queued
os-justin wants to merge 4 commits into
Queued
fix(app-shell): render InspectorSelectField's placeholder, unreachable at all 45 call sites#8494os-justin wants to merge 4 commits into
os-justin wants to merge 4 commits into
Conversation
…e at all 45 call sites The field bridges a caller's "" through an internal sentinel so a "- None -" option can exist at all (Radix `<Select.Item value="">` 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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YBWFb5YgMU5dw8p2VKj16S
Conflict in FlowNodeInspector.declaredDefault.test.tsx's docblock only, and only in its point 1: PR #8473 (objectui#8451) rewrote the BOOLEAN half of that paragraph while this branch rewrote its SELECT half. Resolved by taking the incoming boolean text verbatim and re-applying the select correction on top of it - the select now draws InspectorSelectField's own em-dash placeholder rather than a blank trigger, which is the same fact about `defaultValue` as before. The file's select assertions merged cleanly and are unchanged by this commit. 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
|
Collaborator
Author
|
PM contract review — accepted, flipped to ready, auto-merge armed. Six corrections, two of them against the card I wrote and one against my own instruction. Taking them all. B is right, and the two options were never equivalentI offered "if they come out equivalent on cost, pick and say why". They are not: A makes the Radix Select uncontrolled while the field is empty, because
B measured 0 warnings on both transitions. That is a cost comparison, not a preference.
|
os-justin
marked this pull request as ready for review
September 8, 2026 03:45
os-justin
enabled auto-merge
September 8, 2026 03:45
Collaborator
Author
|
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. Which half objected:
📦 Bundle Size Report
Size Limits
|
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. Which half objected:
📦 Bundle Size Report
Size Limits
|
os-justin
added this pull request to the merge queue
Sep 8, 2026
Any commits made after this event will not be merged.
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 #8450
InspectorSelectFielddeclaresplaceholder = '—'and hands it toSelectValue. It could never render. The field bridges the caller's valuethrough
toInner(value ?? ''), so empty/undefined becomes the sentinel__inspector_select_none__— and Radix showsSelectValue's placeholder forexactly two values,
''andundefined(shouldShowPlaceholder), neither ofwhich the bridge can produce. A controlled value matching no
SelectItemrenders as nothing at all, so every empty select in the metadata-admin designer
drew a blank trigger.
Reproduced before changing anything, rendering the primitive directly:
textContentvalue={undefined}""value="" placeholder="PICKME"""value="profile"(option exists)"Profile"— the lit control: the field IS renderingWhat contradicts the brief
The call-site count is 45, not 61, and 0 of them pass a placeholder.
Measured on
origin/main@c90395b20with a brace-balanced JSX scan over every.tsxin the repo: 51 JSX elements whose opening tag namesInspectorSelectField, 45 outside testfiles, in 15 files. The dispatch asked which call sites pass an explicit
placeholder— the answer is none: the only explicit placeholder anywhere inthe tree is
"Pick one"inside_shared.select.test.tsxitself. So the samplingquestion the brief posed ("which pass a placeholder / which rely on the default")
has a degenerate answer, and the useful split turned out to be a different one
(below). What was unreachable was the primitive's own
'—'default, at all 45.Which repair, and the measured cost of each
Both candidates need the identical predicate (
does any option stand for "none"?), so they cost the same to write and change exactly the same set of callsites. They differ on one thing, and it is decisive.
A — hand Radix
undefinedwhen there is no value. Works (the trigger draws'—', and Radix even setsdata-placeholderitself). Butundefinedis howRadix spells uncontrolled, so the field becomes uncontrolled for exactly as
long as it is empty. Measured against the real
Select, with aconsole.warnspy:
undefined→"profile"): 1 warning,Select is changing from uncontrolled to controlled. …. That is every callsite's first interaction.
""row(
options=[]→[{value:''},…]withvalue=""throughout): 1 warning,same text. The async pickers do this routinely —
useMetaOptions,datasetOptions,chartXOptions,fieldOptionsall populate after mount.useControllableStatekeeps Radix's own value(
setUncontrolledProp), so a pick the owner declines to persist stays onscreen. (Read from the installed source, not rendered.)
B — keep the sentinel, render the placeholder outside
SelectValue. Samemeasurements: 0 warnings on either transition, selected label still renders
across the empty→selected swap,
""-option round trip intact.They are not equivalent on cost — B is strictly cheaper — so B is
implemented. Two details B needs:
SelectValueas children: Radix'sSelectItemTextonly portals the selected label into the value node when!valueNodeHasChildren, so givingSelectValuechildren would silence theselected label. It is rendered as the trigger's child instead of
SelectValue.data-placeholder=""is set on the trigger in the empty state. Not decoration:data-[placeholder]:text-muted-foregroundon the Shadcn trigger is how theempty state is greyed, Radix cannot derive the attribute through the sentinel,
and
SelectPrimitive.Triggerspreads caller props after its own attributes,so the override is legitimate rather than a fight.
The predicate is deliberately narrow — no value AND no option standing for
none. A non-empty value matching no option still renders blank; that is a stale
value, not an empty one, and calling it "—" would assert "nothing is stored"
about a field that is storing something. Filed separately as #8488.
Representative sample, chosen by measurement
The 45 call sites were bucketed mechanically (value expression resolved through
its
constinitializer;optionsresolved to its array literal):""row — empty IS a selection""row—(The static rule put 14 in B;
ActionDefaultInspector.tsx:530is a falsepositive — its
|| 'expression'makes it never empty — and is counted in C byhand. Four C members are "no literal fallback, empirically non-empty" rather than
provably so:
DatasetDefaultInspectord.type/m.aggregate,ObjectFieldInspector:645,ViewColumnInspector:270.)The sample takes one site per bucket, and takes them through the real
inspectors, not the primitive — four inspector components, five triggers:
FlowNodeConfigField.tsx:225http_request"Method",config: {}"""—"AppNavInspector.tsx:391null"""—"PageBlockInspector.tsx:545object-form"Mode""""—"PageBlockInspector.tsx:545object-form"Layout""""—"ObjectFieldInspector.tsx:911""row"— No group —""— No group —"FlowNodeInspector.tsx:401"http_request""http_request"The last two rows are the sample's own controls: A and C must not move, and did
not.
_shared.select.test.tsxcase 3 — reason corrected, case keptIts two assertions were right and are unchanged. Its stated reason was not the
operative one: it read the placeholder's absence as "the
''option matched",but before this change the placeholder was absent in every state, matching
option or not — so the case could not have told a working field from a broken
one. The comment now says that, and points at the neighbouring case where the
very same props with the
""row stripped render"Pick one", which is whatmakes this absence mean what it always claimed to mean. A third assertion was
added: the trigger text is the option label alone.
Would an implementation strictly worse than the bug pass?
No. A field that renders the placeholder always satisfies every positive row,
and fails these two:
does NOT render the placeholder when a value is selected— assertstrigger.textContent === "Profile", not merely that the placeholder is absent,so an implementation rendering both also fails.
does NOT render the placeholder when "" is itself an offered option—asserts
"— No group —".Both live in the same describe as the positives and both stayed green under
the ablation below, which is the asymmetry that makes them load-bearing.
Ablation — the pin can fail, and fails for the right reason
Ablated the read site: restored the pre-fix trigger (a bare
SelectValue placeholder={placeholder}element under an unconditionalSelectTrigger) on top of the committed implementation. Direction predicted before running.Mutation proven on disk, hashes and both grep directions with the matched line:
No rebuild is involved and none is owed: the pins reach the mutated module by the
relative source import
from './_shared', not through a packageexportsentry,so there is no
dist/copy that could serve a stale green.Result —
7 failed | 16 passed (23), red by name:and green throughout on both
does NOT render …rows and all threeempty-value optionrows. Restored by state, not by an exit code:git hash-objectback toccdf0f887245e15fd17e16a12752a8582f65f069(equal tothe HEAD blob) and
git diff HEADempty; re-run after restore23 passed (23).The script carried
trap … EXIT INT TERMwith absolute paths throughout.FlowNodeInspector.declaredDefault.test.tsxTwo rows read
'—'where they read''. That file is objectui#6830's pin andits subject — a declared
defaultValueseeds no control — is untouched; thequeryByText('GET') === null/queryByText('POST') === nullassertions carry itand are unchanged. Only the mark for "nothing is selected" moved from blank to
the placeholder. The select half of #6830's ruled direction A is deliberately
not implemented here — this PR shows "nothing is selected", never the declared
default.
objectui#3912
Read. No collision, no pre-emption. It is about
BlockPropField'stextbranch in
previews/block-config.tscarryingplaceholderas its onlycapability and wanting
pattern/validate/description— aInspectorTextFieldplaceholder, which renders fine (the nativeplaceholderattribute on aninputelement) and is being asked to carry validation semantics it should not.This PR touches neither
block-config.tsnorInspectorTextField, and adds nocapability to
BlockPropField. The one point of contact is that thekind: 'select'block props atPageBlockInspector.tsx:545now show—when unset —which affects nothing #3912 proposes. It is
pm:on-hold.Verification
Everything from the repo root with explicit positional paths.
pnpm --workspace-concurrency=2 --filter '@object-ui/app-shell^...' build— exit 0pnpm exec vitest run packages/app-shell/—Test Files 650 passed (650)·Tests 6281 passed | 1 skipped (6282)pnpm exec vitest run apps/console/ examples/byo-backend-console/ examples/console-starter/—Test Files 90 passed (90)·Tests 1078 passed (1078)(the other three packagesturbo ls --affectedreports; run from the root, never--filter-ed, because a filteredexamples/*run relocatesprocess.cwd())pnpm --filter @object-ui/app-shell run type-check— exit 0 (it istsc --noEmit && tsc -p tsconfig.test.json, so the new test file is type-checked too)pnpm --filter @object-ui/app-shell run lint(eslint ., package-scoped, no narrowing) — exit 0,✖ 2925 problems (0 errors, 2925 warnings). The fivereact-refresh/only-export-componentswarnings on_shared.tsxare pre-existing and shifted, not new: the file's export list is byte-identical toc90395b20(15 exports, same names), and the warnings sit onspliceArray/insertArray/appendArray/moveArray/uniqueId.node scripts/check-changeset-presence.mjs—✅ 3 source file(s) of 1 released package(s) changed, and this change declares 1 changeset(s): .changeset/8450-inspector-select-placeholder.md.node scripts/check-governed-queue-guard.mjs --test THE-4-PATHS—✅ NOT GOVERNEDgrep -naP '[\x00-\x08\x0b\x0c\x0e-\x1f\x7f]'→ no hits (exit 1), with a lit control that fired on a planted\x01.No assertion here depends on
window.innerWidth: nothing opens the Radix content(the popper is the only viewport-sensitive part), every reading is the closed
trigger's
textContentor an attribute, and no responsive class is asserted.Changeset level
patch, for@object-ui/app-shell.InspectorSelectFieldis not exportedfrom any package index — it is internal to the metadata-admin designer, so no
published API moves. What changes is rendered output inside a shipped app
surface, which is what a patch-level fix is.
minorwould claim a newcapability; the
placeholderprop was already declared and documented, it simplynever reached the DOM. (
majoris forbidden repo-wide — one fixed group.)skip-changesetwas not used: it is a phantom label here, and the gate abovenames the real declaration.
its shape spelled out in prose, never as a literal short angle-bracket span:
the GitHub body sanitizer eats those, code fences included, and the eaten line
here would be the one carrying the ablation's injected marker. Session
attribution as durable prose, since footer blocks are rewritten on edit — this
PR was produced by the agent seat at
https://claude.ai/code/session_01YBWFb5YgMU5dw8p2VKj16S.🤖 Generated with Claude Code
https://claude.ai/code/session_01YBWFb5YgMU5dw8p2VKj16S
Generated by Claude Code