fix(select): make renderValue and placeholder work together#413
fix(select): make renderValue and placeholder work together#413mattrothenberg merged 12 commits intomainfrom
Conversation
## Problem
When using Select with object values and `strictNullChecks` enabled in
TypeScript, there were two issues:
1. **Type inference bug**: `T` would collapse to `never` due to
covariant/contravariant conflicts between the `value` prop and
callback props like `renderValue`.
2. **API confusion**: Base UI's `placeholder` prop is ignored when you
provide a children-as-function (which `renderValue` maps to). This
meant users had to handle the null case inside `renderValue`, making
`placeholder` useless when customizing value display.
## Solution
Kumo now wraps the `renderValue` function internally:
- When value is `null`, we render the `placeholder` with appropriate styling
- When value is set, we call `renderValue` with the non-null value
- The type signature remains `(value: T) => ReactNode` (non-null)
This means `placeholder` and `renderValue` work together as users expect:
```tsx
<Select
placeholder="Select a user..."
value={user}
renderValue={(user) => user.name} // Only called when user is not null
/>
```
## Changes
### packages/kumo/src/components/select/select.tsx
- Add JSDoc explaining `renderValue` and `placeholder` interaction
- Wrap `renderValue` to handle null internally, showing placeholder
### packages/kumo-docs-astro/src/components/demos/SelectDemo.tsx
- Update SelectComplexDemo to use `placeholder` + `renderValue` pattern
- Update SelectDisabledOptionsDemo to use object values with `{ value, label }`
- Update SelectGroupedDemo to use object values
- Update SelectGroupedWithDisabledDemo to use object values
- Update SelectLongListDemo to use object values
- All demos now show the label (not raw value) when an item is selected
### packages/kumo-docs-astro/src/pages/components/select.mdx
- Update Placeholder section to explain `renderValue` interaction
- Update Custom Rendering section with clearer guidance
- Update Multiple Selection section with code example
commit: |
|
/bonk I need a thorough review of this one. |
Docs PreviewCommit: |
Visual Regression Report — 18 changed, 19 unchanged18 screenshot(s) with visual changes: Button / Variant: Primary370 px (0.36%) changed
Button / Loading State4 px (0%) changed
Dialog / Dialog With Actions138 px (0.14%) changed
Dialog / Dialog Basic197 px (0.19%) changed
Dialog / Dialog Confirmation352 px (0.35%) changed
Dialog / Dialog With Select225 px (0.22%) changed
Dialog (Open)0 px (0%) changed
Select / Select Sizes947 px (0.51%) changed
Select / Select Placeholder418 px (0.41%) changed
Select / Select With Tooltip500 px (0.49%) changed
Select / Select Custom Rendering414 px (0.41%) changed
Select / Select Loading0 px (0%) changed
Select / Select Multiple0 px (0%) changed
Select / Select Complex1,090 px (0.92%) changed
Select / Select Disabled Options0 px (0%) changed
Select / Select Grouped194 px (0.19%) changed
Select / Select Long List1,428 px (1.21%) changed
Select (Open)1,461,715 px (5.89%) changed
19 screenshot(s) unchanged
Generated by Kumo Visual Regression |
|
UnknownError: ProviderInitError |
|
@geoquant Bonk workflow failed. Check the logs for details. View workflow run · To retry, trigger Bonk again. |
…tests - Remove Select.Value compound component (renderValue prop covers all use cases) - Simplify valueChildrenFn logic (extract to variable for clarity) - Add stress test page at /tests/select with 12 test scenarios - Add TypeScript type tests with strictNullChecks enabled - All tests verify renderValue/placeholder interaction works correctly
|
/bonk review |
|
UnknownError: ProviderInitError |
|
/bigbonk |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: fix(select): make renderValue and placeholder work together
Good fix for a real pain point. The core logic change is small and well-targeted. A few issues to address:
Issues
1. as never cast is a code smell
At select.tsx:409:
return renderValue(value as never);never is assignable to anything, so this suppresses all type checking. The value flowing in is unknown (from Base UI's children callback), and renderValue expects Multiple extends true ? T[] : T. A safer cast would be:
return renderValue(value as Multiple extends true ? T[] : T);Or since the generic isn't available at that scope, at minimum use as any which is more conventional for deliberate type boundary crossings. as never reads like an accident.
2. renderValue + no placeholder renders an empty span
When renderValue is provided but placeholder is not, and the value is null, line 407 renders:
<span className="text-kumo-placeholder">{undefined}</span>This produces an empty <span> in the DOM where Base UI's native placeholder handling would have shown nothing. Consider falling back to Base UI's default behavior when no placeholder is specified:
if (value == null) {
if (placeholder != null) {
return <span className="text-kumo-placeholder">{placeholder}</span>;
}
return null; // Let Base UI handle it
}Though returning null from the children function still bypasses Base UI's placeholder logic (since the function was called). You may need to return undefined or test what Base UI does when the function returns null. Worth verifying.
3. PR description claims "Remove Select.Value compound component" but it wasn't removed
The PR description says "Remove Select.Value compound component (renderValue prop covers all use cases)" but Select.Value was never a public compound component — it's Base UI's internal SelectBase.Value. The description is misleading here and should be corrected.
Type Tests
The type test file (select-strict-types.test-d.tsx) is well structured. One note:
At line 109, TestRenderValueNonNull uses implicit generic inference:
<Select
value={country}
onValueChange={setCountry}
placeholder="Select..."
renderValue={(c) => { ... }}
>This is the scenario that originally broke. Can you confirm this actually compiles under the strict tsconfig? The onValueChange={setCountry} line is the one that would cause T to infer as never in the original bug — setCountry accepts { code: string; name: string } | null but renderValue expects T (not T | null). If the fix is purely at the runtime level (wrapping the function), the original TS inference issue with strictNullChecks may still exist for users who don't use explicit <Select<T>> syntax.
I'd want to see this confirmed: does TestRenderValueNonNull pass under tsconfig.strict.json without the explicit <Select<Country>> generic?
Stress Test Page
The stress test page at /tests/select is thorough — 12 scenarios cover the major API surface. Nice addition for manual verification.
Minor
- The changeset is well-written and documents the behavioral contract clearly.
- The
renderedChildrenrefactor from??to ternary is fine but is a no-op change (same semantics). Not a problem, just noting it's cosmetic.
Co-authored-by: mattrothenberg <mattrothenberg@users.noreply.github.com>
|
The review has been posted. Here's a summary of my review: Review Summary for PR #413What the PR doesFixes two issues with the Select component:
The fix wraps Issues raised
|
- Change 'as never' to 'as any' with comment explaining type boundary - Return null instead of empty span when no placeholder provided - Update PR description (removed incorrect claim about Select.Value)
Confirms that WITHOUT explicit <Select<T>> generic: - onValueChange receives T | null - renderValue receives T (non-null) - isItemEqualToValue receives correct types These replicate the exact pattern that broke in stratus.
|
/bigbonk OK I made the changes you requested |
There was a problem hiding this comment.
Review: fix(select): make renderValue and placeholder work together
I've reviewed the full PR including the core fix, type tests, stress test page, demos, and documentation. I also verified the type tests compile against the strict tsconfig.
Core Fix Assessment
The fix is well-targeted. The approach of wrapping renderValue to intercept null values is the right call given Base UI's design — their SelectValue explicitly documents that placeholder is "overridden by children if specified" (see SelectValue.d.ts:34). There's no way around this without wrapping.
The implementation at select.tsx:404-416 is clean:
placeholder == null→ returnnull(no empty DOM node)placeholder != null→ render placeholder span withtext-kumo-placeholder- Non-null value → delegate to user's
renderValue
Bonk's Issues — Status
Both code-level issues from bonk's review have been addressed in commit 265b516:
as never→as any: Fixed, with a comment explaining the deliberate type boundary. This is the right cast —as anyis the conventional escape hatch for intentional generic boundaries, and the comment makes it clear this isn't accidental.- Empty span when no placeholder: Fixed by adding the
placeholder == nullguard that returnsnull. No empty<span>will render.
Type Tests
I ran tsc --project packages/kumo/tests/tsconfig.strict.json --noEmit and confirmed zero type errors (excluding the known ai/schemas stub that's auto-generated at build time).
The critical test is stratus-scenario.test-d.tsx — it replicates the exact pattern that broke in production, without explicit <Select<T>> generics:
const [tunnel, setTunnel] = useState<Tunnel | null>(null);
<Select
value={tunnel}
onValueChange={setTunnel}
renderValue={(t) => t.name} // T inferred correctly, not never
>This compiles cleanly under strictNullChecks + strictFunctionTypes. The inference works because renderValue's type is (value: Multiple extends true ? T[] : T) => ReactNode — when T is Tunnel, renderValue expects (value: Tunnel) => ReactNode, which doesn't conflict with onValueChange's (value: Tunnel | null) => void. The | null only appears on value and onValueChange, not on renderValue.
One Observation
The renderedChildren refactor from ?? to ternary (select.tsx:395-399) is semantically equivalent — ?? would have the same behavior here since children is ReactNode and 0/"" are valid ReactNode values that shouldn't fall through. The ternary is arguably more readable since it makes the three-way branch explicit, so this is fine.
Stress Test Page
The /tests/select page with 12 scenarios is a good addition for manual regression testing. Each scenario isolates a specific API interaction (strictNullChecks, renderValue+placeholder, items array, object map, multiple, controlled/uncontrolled, groups, field integration, sizes, loading, disabled, inference).
Summary
Approve the direction. The two bonk issues have been addressed. The type tests confirm the inference fix works under strict mode without explicit generics. The runtime wrapping correctly handles the Base UI placeholder limitation.
|
Review posted on PR #413. Here's a summary of my findings: The fix is solid. The core change wraps Both bonk review issues have been addressed in commit
Type tests pass under |
2 participants
Summary
This PR fixes two issues with the Select component:
TypeScript inference bug - Under
strictNullChecks, usingvalue={objectOrNull}would causeTto be inferred asnever, making callback props likerenderValueandonValueChangeunusable.placeholderignored when usingrenderValue- Base UI's design means theplaceholderprop is completely ignored when you provide a children-as-function (whichrenderValuemaps to). Users had to handle the null case insiderenderValue, defeating the purpose ofplaceholder.The Fix
Kumo now wraps the
renderValuefunction internally to:placeholder(with proper styling) when value is nullrenderValuewith non-null valuesplaceholderis provided and value is null, returnnull(show nothing)Changes
packages/kumo/src/components/select/select.tsxrenderValueto handle null internally, showing placeholder with proper stylingpackages/kumo-docs-astro/placeholder+renderValuepattern/tests/selectwith 12 scenariospackages/kumo/tests/strictNullChecksandstrictFunctionTypesenabledStress Test Page
Visit
/tests/selectto see all API permutations:Testing
<Select<T>>generic syntaxplaceholdershows when value is nullrenderValueis only called with non-null values