feat(react-ui-base): add form compound component#2270
feat(react-ui-base): add form compound component#2270lachieh wants to merge 2 commits intolachieh/tam-1057-message-suggestionsfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
The biggest correctness risk is state updates in FormRoot using spread-based writes from a captured state, which can lose updates under rapid interactions; switching to functional updates is important. Slider support currently ignores sliderMin/sliderStep and can persist "value : undefined" when label indices drift, both of which will surface as real user bugs. Validation is minimal, allowing misconfigured fields to render without controls, and the click-outside handler can cause multiple redundant state writes per click. The UI Registry wrapper also leaves several field types effectively unstyled by delegating to the base defaults.
Additional notes (1)
- Maintainability |
packages/ui-registry/src/components/form/form.tsx:250-285
In the styled wrapper,StyledSelectInput/StyledRadioInput/StyledCheckboxInput/StyledSliderInput/StyledYesNoInputall just render<FormBase.FieldInput field={field} />, which delegates to the base default UI. That default UI currently has no Tailwind styling, so those field types will look unstyled compared to text/number/textarea.
If the goal of ui-registry is a consistently styled component, you should either:
- provide styled children for these cases (using the base primitives/state), or
- style the base defaults via
[data-slot=...]selectors in CSS so the base renderer becomes presentationally acceptable.
As-is, the showcase will likely look inconsistent.
Summary of changes
Added new Form compound component to @tambo-ai/react-ui-base
- New entrypoint export: added
"./form"topackages/react-ui-base/package.jsonexports, pointing todist/esm/form+dist/cjs/form. - New base primitives under
packages/react-ui-base/src/form/*:Form.Rootwith context/state management viauseTamboComponentStateForm.Fieldsrender-prop containerForm.Fieldplus subpartsFieldLabel,FieldDescription,FieldInputForm.Submitrender-prop buttonForm.Errorconditional form-level error display
- Public API wiring: exported
Form+ related types frompackages/react-ui-base/src/index.ts.
Updated UI Registry styled Form implementation
packages/ui-registry/src/components/form/form.tsxnow composes the new base primitives from@tambo-ai/react-ui-base/forminstead of duplicating state/handlers.- Added
@tambo-ai/react-ui-base@nextto the registry componentconfig.jsondependencies. - Introduced styled wrappers (
StyledField,StyledFieldInput, etc.) and kept the submit loading state viaFormBase.Submitrender-prop.
| const handleDropdownToggle = React.useCallback( | ||
| (fieldId: string) => { | ||
| if (!state) return; | ||
| setState({ | ||
| ...state, | ||
| openDropdowns: { | ||
| ...state.openDropdowns, | ||
| [fieldId]: !state.openDropdowns[fieldId], | ||
| }, | ||
| }); | ||
| }, | ||
| [state, setState], | ||
| ); | ||
|
|
||
| const handleOptionSelect = React.useCallback( | ||
| (fieldId: string, option: string) => { | ||
| if (!state) return; | ||
| setState({ | ||
| ...state, | ||
| selectedValues: { | ||
| ...state.selectedValues, | ||
| [fieldId]: option, | ||
| }, | ||
| openDropdowns: { | ||
| ...state.openDropdowns, | ||
| [fieldId]: false, | ||
| }, | ||
| }); | ||
| }, | ||
| [state, setState], | ||
| ); | ||
|
|
||
| const handleYesNoSelection = React.useCallback( | ||
| (fieldId: string, value: string) => { | ||
| if (!state) return; | ||
| setState({ | ||
| ...state, | ||
| yesNoSelections: { | ||
| ...state.yesNoSelections, | ||
| [fieldId]: value, | ||
| }, | ||
| values: { | ||
| ...state.values, | ||
| [fieldId]: value, | ||
| }, | ||
| }); | ||
| }, | ||
| [state, setState], | ||
| ); | ||
|
|
||
| const handleCheckboxChange = React.useCallback( | ||
| (fieldId: string, option: string, checked: boolean) => { | ||
| if (!state) return; | ||
|
|
||
| const currentSelections = state.checkboxSelections[fieldId] ?? []; | ||
|
|
||
| const newSelections = checked | ||
| ? [...currentSelections, option] | ||
| : currentSelections.filter((item) => item !== option); | ||
|
|
||
| setState({ | ||
| ...state, | ||
| checkboxSelections: { | ||
| ...state.checkboxSelections, | ||
| [fieldId]: newSelections, | ||
| }, | ||
| values: { | ||
| ...state.values, | ||
| [fieldId]: newSelections.join(","), | ||
| }, | ||
| }); | ||
| }, | ||
| [state, setState], | ||
| ); | ||
|
|
||
| const handleSliderChange = React.useCallback( | ||
| (fieldId: string, value: string, field: FormFieldDefinition) => { | ||
| if (!state) return; | ||
|
|
||
| const label = | ||
| field.sliderLabels && field.sliderLabels.length > 0 | ||
| ? field.sliderLabels[parseInt(value)] | ||
| : value; | ||
|
|
||
| setState({ | ||
| ...state, | ||
| values: { | ||
| ...state.values, | ||
| [fieldId]: `${value} : ${label}`, | ||
| }, | ||
| }); | ||
| }, | ||
| [state, setState], | ||
| ); | ||
|
|
There was a problem hiding this comment.
handleDropdownToggle, handleOptionSelect, handleYesNoSelection, handleCheckboxChange, and handleSliderChange all read from the captured state and then call setState({ ...state, ... }). This is fragile under rapid interactions because multiple events can be processed against a stale snapshot, causing lost updates (classic non-functional setState race).
You already added stateRef for click-outside; consider using the functional updater form consistently for all these handlers, and you can remove most if (!state) return branches. This will make updates correct even if useTamboComponentState batches or if events fire quickly (e.g., checkbox toggles + dropdown closing).
Suggestion
Switch to functional updates for all state transitions to avoid stale-state races.
const handleDropdownToggle = React.useCallback((fieldId: string) => {
setState(prev => ({
...prev,
openDropdowns: {
...prev.openDropdowns,
[fieldId]: !prev.openDropdowns[fieldId],
},
}));
}, [setState]);
const handleCheckboxChange = React.useCallback(
(fieldId: string, option: string, checked: boolean) => {
setState(prev => {
const currentSelections = prev.checkboxSelections[fieldId] ?? [];
const newSelections = checked
? Array.from(new Set([...currentSelections, option]))
: currentSelections.filter(v => v !== option);
return {
...prev,
checkboxSelections: {
...prev.checkboxSelections,
[fieldId]: newSelections,
},
values: {
...prev.values,
[fieldId]: newSelections.join(","),
},
};
});
},
[setState],
);Apply the same pattern to option/yes-no/slider updates and the click-outside closer.
Reply with "@CharlieHelps yes please" if you'd like me to add a commit with this suggestion.
| const handleSliderChange = React.useCallback( | ||
| (fieldId: string, value: string, field: FormFieldDefinition) => { | ||
| if (!state) return; | ||
|
|
||
| const label = | ||
| field.sliderLabels && field.sliderLabels.length > 0 | ||
| ? field.sliderLabels[parseInt(value)] | ||
| : value; | ||
|
|
||
| setState({ | ||
| ...state, | ||
| values: { | ||
| ...state.values, | ||
| [fieldId]: `${value} : ${label}`, | ||
| }, | ||
| }); | ||
| }, | ||
| [state, setState], | ||
| ); | ||
|
|
||
| const getDefaultSliderValue = React.useCallback( | ||
| (field: FormFieldDefinition): string => { | ||
| const defaultVal = | ||
| field.sliderDefault?.toString() ?? | ||
| (field.sliderLabels && field.sliderLabels.length > 0 | ||
| ? Math.floor((field.sliderLabels.length - 1) / 2).toString() | ||
| : "5"); | ||
|
|
||
| const defaultLabel = | ||
| field.sliderLabels && field.sliderLabels.length > 0 | ||
| ? field.sliderLabels[parseInt(defaultVal)] | ||
| : defaultVal; | ||
|
|
||
| return `${defaultVal} : ${defaultLabel}`; | ||
| }, | ||
| [], | ||
| ); |
There was a problem hiding this comment.
handleSliderChange and getDefaultSliderValue use parseInt(value) to index sliderLabels without bounds checking. If the slider value is ever out of range (e.g., labels change between renders, persisted state from a previous field definition, or a user agent sending an unexpected value), you’ll format values like "3 : undefined" and persist that.
Given this state is persisted via useTamboComponentState keyed by a derived formId, it’s realistic to see out-of-date values after a schema update.
Suggestion
Clamp the label index and fall back to the raw value when out of range.
const idx = Number(value);
const label = field.sliderLabels?.length
? field.sliderLabels[Math.min(Math.max(idx, 0), field.sliderLabels.length - 1)] ?? value
: value;Apply the same clamping in getDefaultSliderValue.
Reply with "@CharlieHelps yes please" if you'd like me to add a commit with this suggestion.
| const validFields = React.useMemo(() => { | ||
| return fields.filter((field): field is FormFieldDefinition => { | ||
| if (!field || typeof field !== "object") { | ||
| console.warn("Invalid field object detected"); | ||
| return false; | ||
| } | ||
| if (!field.id || typeof field.id !== "string") { | ||
| console.warn("Field missing required id property"); | ||
| return false; | ||
| } | ||
| return true; | ||
| }); | ||
| }, [fields]); | ||
|
|
There was a problem hiding this comment.
validFields only validates field.id and that the object exists. But the base FormFieldInputDefault assumes type-specific required properties (e.g., options for select/radio/checkbox) and silently returns null when missing.
This creates a failure mode where the form renders a label/description but no control, and still may be required (e.g., select with required: true but options: undefined). That’s a correctness + UX issue that should be caught early.
Consider validating type and required per-type config in validFields (or surfacing a consistent error slot) so consumers can detect misconfigured schemas.
Suggestion
Expand field validation to enforce basic invariants per field.type.
Example approach:
const validFields = React.useMemo(() => {
return fields.filter((field): field is FormFieldDefinition => {
if (!field || typeof field !== "object") return false;
if (!field.id || typeof field.id !== "string") return false;
if (["select","radio","checkbox"].includes(field.type)) {
if (!Array.isArray(field.options) || field.options.length === 0) return false;
}
if (field.type === "slider") {
if (!field.sliderLabels?.length) {
// optional: ensure min/max/step sanity
}
}
return true;
});
}, [fields]);Optionally emit console.warn with enough context (field.id, field.type) when filtering.
Reply with "@CharlieHelps yes please" if you'd like me to add a commit with this suggestion.
| React.useEffect(() => { | ||
| const handleClickOutside = (event: MouseEvent) => { | ||
| Object.entries(dropdownRefs.current).forEach(([fieldId, ref]) => { | ||
| if (ref && !ref.contains(event.target as Node) && stateRef.current) { | ||
| setState({ | ||
| ...stateRef.current, | ||
| openDropdowns: { | ||
| ...stateRef.current.openDropdowns, | ||
| [fieldId]: false, | ||
| }, | ||
| }); | ||
| } | ||
| }); | ||
| }; | ||
|
|
||
| document.addEventListener("mousedown", handleClickOutside); | ||
| return () => | ||
| document.removeEventListener("mousedown", handleClickOutside); | ||
| }, [setState]); |
There was a problem hiding this comment.
The click-outside handler iterates over all dropdown refs and calls setState for each dropdown that is open/has a ref and the click is outside it. This can trigger multiple state writes for a single click and can also re-close dropdowns repeatedly even when already closed.
This is both unnecessary work and can cause extra renders. It also relies on the stateRef snapshot and spreads it each time, which is another place where functional updates would simplify.
Better: compute whether any dropdown needs closing and perform a single setState that closes all open dropdowns at once (or only the ones that should close).
Suggestion
Batch dropdown closing into a single state update.
React.useEffect(() => {
const handleClickOutside = (event: MouseEvent) => {
const target = event.target as Node;
setState(prev => {
let changed = false;
const nextOpen = { ...prev.openDropdowns };
for (const [fieldId, el] of Object.entries(dropdownRefs.current)) {
if (!el) continue;
if (!el.contains(target) && nextOpen[fieldId]) {
nextOpen[fieldId] = false;
changed = true;
}
}
return changed ? { ...prev, openDropdowns: nextOpen } : prev;
});
};
document.addEventListener("mousedown", handleClickOutside);
return () => document.removeEventListener("mousedown", handleClickOutside);
}, [setState]);Reply with "@CharlieHelps yes please" if you'd like me to add a commit with this suggestion.
| /** | ||
| * Slider/range field. | ||
| */ | ||
| function FormFieldSlider({ field }: { field: FormFieldDefinition }) { | ||
| const { state, handleSliderChange, getDefaultSliderValue } = | ||
| useFormRootContext(); | ||
|
|
||
| const maxValue = | ||
| field.sliderLabels && field.sliderLabels.length > 0 | ||
| ? (field.sliderLabels.length - 1).toString() | ||
| : (field.sliderMax ?? 10).toString(); | ||
|
|
||
| const currentValue = | ||
| state.values[field.id]?.split(" : ")[0] ?? | ||
| field.sliderDefault?.toString() ?? | ||
| (field.sliderLabels && field.sliderLabels.length > 0 | ||
| ? Math.floor((field.sliderLabels.length - 1) / 2).toString() | ||
| : "5"); | ||
|
|
||
| return ( | ||
| <div data-slot="form-field-slider"> | ||
| <input | ||
| type="range" | ||
| id={`${field.id}-range`} | ||
| min="0" | ||
| max={maxValue} | ||
| step="1" | ||
| value={currentValue} | ||
| required={field.required} | ||
| data-slot="form-field-slider-input" | ||
| onChange={(e) => handleSliderChange(field.id, e.target.value, field)} | ||
| /> | ||
| <input | ||
| type="hidden" | ||
| name={field.id} | ||
| value={state.values[field.id] ?? getDefaultSliderValue(field)} | ||
| /> | ||
| {field.sliderLabels && field.sliderLabels.length > 0 ? ( | ||
| <div data-slot="form-field-slider-labels"> | ||
| {field.sliderLabels.map((label, index) => ( | ||
| <span key={index}>{label}</span> | ||
| ))} | ||
| </div> | ||
| ) : ( | ||
| <div data-slot="form-field-slider-labels"> | ||
| <span>Min</span> | ||
| <span>Mid</span> | ||
| <span>Max</span> | ||
| </div> | ||
| )} | ||
| </div> | ||
| ); |
There was a problem hiding this comment.
FormFieldSlider ignores sliderMin and sliderStep from the field definition. It hard-codes min="0" and step="1", which is a correctness bug if callers configure a non-zero min or a different step.
Also, maxValue mixes two concepts: if sliderLabels exist, you use label index as the slider value (fine), but you still compute max from sliderMax when labels are absent while ignoring sliderMin/step.
This will produce incorrect behavior and submitted values for configured sliders.
Suggestion
Respect sliderMin, sliderMax, and sliderStep when sliderLabels are not provided. When labels are provided, keep the index-based slider but ensure defaults/min/max align.
const hasLabels = !!field.sliderLabels?.length;
const min = hasLabels ? 0 : (field.sliderMin ?? 0);
const max = hasLabels ? field.sliderLabels!.length - 1 : (field.sliderMax ?? 10);
const step = hasLabels ? 1 : (field.sliderStep ?? 1);
<input
type="range"
min={String(min)}
max={String(max)}
step={String(step)}
// ...
/>You’ll also want to update getDefaultSliderValue() / current value derivation to use sliderMin/max when labels are absent.
Reply with "@CharlieHelps yes please" if you'd like me to add a commit with this suggestion.
| /** | ||
| * Checkbox group field. | ||
| */ | ||
| function FormFieldCheckbox({ field }: { field: FormFieldDefinition }) { | ||
| const { state, handleCheckboxChange } = useFormRootContext(); | ||
|
|
||
| if (!field.options) return null; | ||
|
|
||
| return ( | ||
| <div data-slot="form-field-checkbox"> | ||
| {field.options.map((option) => { | ||
| const selections = state.checkboxSelections[field.id] ?? []; | ||
| const isChecked = selections.includes(option); | ||
|
|
||
| return ( | ||
| <label key={option} data-slot="form-field-checkbox-option"> | ||
| <input | ||
| type="checkbox" | ||
| id={`${field.id}-${option}`} | ||
| checked={isChecked} | ||
| value={option} | ||
| onChange={(e) => | ||
| handleCheckboxChange(field.id, option, e.target.checked) | ||
| } | ||
| /> | ||
| <span>{option}</span> | ||
| </label> | ||
| ); | ||
| })} | ||
| <input | ||
| type="hidden" | ||
| name={field.id} | ||
| value={state.checkboxSelections[field.id]?.join(",") ?? ""} | ||
| /> | ||
| </div> | ||
| ); | ||
| } |
There was a problem hiding this comment.
FormFieldCheckbox renders controlled checkboxes (checked={isChecked}) but does not set name on each checkbox input; instead it uses a hidden input to submit a comma-joined list. That’s OK for submission, but the visible inputs will not participate in native form submission, and the required attribute cannot work as expected for checkbox groups (it won’t be applied to the hidden input either).
If you intend native validation, you need a different strategy (e.g., a single required hidden input that updates to non-empty when any selection exists and is marked required, or per-checkbox name with required only on one and custom validation).
Suggestion
If field.required is meant to enforce at least one checkbox selection, apply required to the hidden input and ensure its value becomes non-empty when any option is checked.
<input
type="hidden"
name={field.id}
value={state.checkboxSelections[field.id]?.join(",") ?? ""}
required={field.required}
/>If you want native checkbox submission semantics instead, add name={field.id} to each checkbox and remove the hidden input.
Reply with "@CharlieHelps yes please" if you'd like me to add a commit with this suggestion.
| export const FormFieldLabel = React.forwardRef< | ||
| HTMLLabelElement, | ||
| FormFieldLabelProps | ||
| >(function FormFieldLabel({ field, asChild, children, ...props }, ref) { | ||
| const Comp = asChild ? Slot : "label"; | ||
|
|
||
| return ( | ||
| <Comp ref={ref} data-slot="form-field-label" htmlFor={field.id} {...props}> | ||
| {children ?? ( | ||
| <> | ||
| {field.label} | ||
| {field.required && <span data-slot="form-field-required">*</span>} | ||
| </> | ||
| )} | ||
| </Comp> | ||
| ); | ||
| }); |
There was a problem hiding this comment.
FormFieldLabel always passes htmlFor={field.id} even when asChild is used. If the consumer uses asChild to render a non-<label> element, you’ll end up applying htmlFor to an incompatible element (and TypeScript won’t save you because Slot is permissive).
Similarly, the default rendering includes a required * indicator without any aria semantics. At minimum, the star should be aria-hidden and required-ness should be conveyed via the input’s required attribute (which you already do).
Suggestion
Only apply htmlFor when rendering a real label, or allow overriding via props.
const Comp = asChild ? Slot : "label";
return (
<Comp
ref={ref}
data-slot="form-field-label"
{...(!asChild ? { htmlFor: field.id } : null)}
{...props}
>
{children ?? (
<>
{field.label}
{field.required && (
<span data-slot="form-field-required" aria-hidden="true">*</span>
)}
</>
)}
</Comp>
);Reply with "@CharlieHelps yes please" if you'd like me to add a commit applying this.
| const currentValue = | ||
| state.values[field.id]?.split(" : ")[0] ?? | ||
| field.sliderDefault?.toString() ?? | ||
| (field.sliderLabels && field.sliderLabels.length > 0 | ||
| ? Math.floor((field.sliderLabels.length - 1) / 2).toString() | ||
| : "5"); |
There was a problem hiding this comment.
The slider value formatting uses the string delimiter " : " and later parses with split(" : "). This is fragile: labels can reasonably contain : which would corrupt parsing, and it couples display formatting to state storage.
Storing a structured value (e.g., raw numeric value + derived label) avoids this. If you need to submit both, derive the display string at submit time or store separate fields.
Suggestion
Avoid encoding multiple pieces of data into a single delimited string. Minimal change: store the numeric value in state.values[fieldId] and derive the label for rendering.
Example approach:
// state.values[fieldId] = value
setState({
...s,
values: { ...s.values, [fieldId]: value },
});
const label = field.sliderLabels?.[Number(currentValue)] ?? currentValue;If you need to submit the label too, submit value and derive label server-side or add a second hidden input (e.g., ${field.id}Label). Reply with "@CharlieHelps yes please" if you'd like me to add a commit with a safe representation.
3d02498 to
b753c1c
Compare
62d9466 to
db3b91a
Compare
b753c1c to
c07cab3
Compare
Fixes TAM-1051 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
… render prop in form
db3b91a to
8281b73
Compare
Summary
Adds
formcompound component base primitives and styled wrapper.Base: Root, Fields, Field, Submit, Error
Styled: Composes base components with Tailwind styling
Fixes TAM-1051
Test Plan
npm run lint && npm run check-types && npm test🤖 Generated with Claude Code