feat(react-ui-base): add elicitation-ui compound component#2271
feat(react-ui-base): add elicitation-ui compound component#2271lachieh wants to merge 4 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 issue is that ElicitationUIRoot does not reset formData/touchedFields when request changes, which can leak stale values into subsequent submissions. isValid is also recalculated eagerly and does repeated requiredFields.includes scans; memoizing and using a Set would reduce unnecessary work. Single-entry mode currently bypasses the same validation guard used for explicit submit, creating inconsistent behavior across modes. The styled registry wrapper should type schema precisely rather than erasing the schema shape.
Summary of changes
What changed
@tambo-ai/react-ui-base
- Added a new export path entry for
./elicitation-uiinpackages/react-ui-base/package.json. - Introduced a new compound/namespace-style base component set under
packages/react-ui-base/src/elicitation-ui/*:Rootwith context/state/validationTitleFielddispatcherStringField,NumberField,BooleanField,EnumFieldActions- Shared validation helpers in
root/validation.ts
- Re-exported
ElicitationUIBaseand associated types/helpers frompackages/react-ui-base/src/index.ts.
@tambo-ai/ui-registry
- Updated registry config to depend on
@tambo-ai/react-ui-base. - Refactored the styled
ElicitationUIto compose the new base primitives (ElicitationUIBase.*) and provide Tailwind styling via render props, removing duplicated state/validation logic from the registry implementation.
| const singleEntry = isSingleEntryMode(request); | ||
|
|
||
| const fields = React.useMemo( | ||
| () => Object.entries(request.requestedSchema.properties), | ||
| [request.requestedSchema.properties], | ||
| ); | ||
|
|
||
| const requiredFields = React.useMemo( | ||
| () => request.requestedSchema.required ?? [], | ||
| [request.requestedSchema.required], | ||
| ); | ||
|
|
||
| const [formData, setFormData] = React.useState<Record<string, unknown>>( | ||
| () => { | ||
| const initial: Record<string, unknown> = {}; | ||
| fields.forEach(([name, schema]) => { | ||
| if (schema.default !== undefined) { | ||
| initial[name] = schema.default; | ||
| } | ||
| }); | ||
| return initial; | ||
| }, | ||
| ); | ||
|
|
There was a problem hiding this comment.
formData is initialized from fields once (via the lazy initializer), but there is no reset when request (and therefore fields) changes. If the component is reused for a new request instance, the UI can show stale values and submit the wrong content.
This is especially likely in chat-style UIs where you receive multiple elicitation requests over time using the same mounted component tree.
Suggestion
Reset formData/touchedFields when the request schema changes (or key the root by request id if available). For example:
const fields = React.useMemo(
() => Object.entries(request.requestedSchema.properties),
[request.requestedSchema.properties],
);
React.useEffect(() => {
const initial: Record<string, unknown> = {};
for (const [name, schema] of fields) {
if (schema.default !== undefined) initial[name] = schema.default;
}
setFormData(initial);
setTouchedFields(new Set());
}, [fields]);Reply with "@CharlieHelps yes please" if you'd like me to add a commit with this suggestion.
| const requiredFields = React.useMemo( | ||
| () => request.requestedSchema.required ?? [], | ||
| [request.requestedSchema.required], | ||
| ); | ||
|
|
||
| const [formData, setFormData] = React.useState<Record<string, unknown>>( | ||
| () => { | ||
| const initial: Record<string, unknown> = {}; | ||
| fields.forEach(([name, schema]) => { | ||
| if (schema.default !== undefined) { | ||
| initial[name] = schema.default; | ||
| } | ||
| }); | ||
| return initial; | ||
| }, | ||
| ); | ||
|
|
||
| const [touchedFields, setTouchedFields] = React.useState<Set<string>>( | ||
| new Set(), | ||
| ); | ||
|
|
||
| const isValid = fields.every(([fieldName, fieldSchema]) => { | ||
| const value = formData[fieldName]; | ||
| const isRequired = requiredFields.includes(fieldName); | ||
| return validateField(value, fieldSchema, isRequired).valid; | ||
| }); | ||
|
|
There was a problem hiding this comment.
isValid recomputes on every render and does an includes scan over requiredFields for each field, making it O(nm)*. For larger schemas this is avoidable work, and it also means validateField runs even when nothing relevant changed.
Since this value drives button disabled state and submission guard, it’s worth memoizing and using a Set for required lookups.
Suggestion
Memoize isValid and precompute a Set for required lookups:
const requiredSet = React.useMemo(
() => new Set(request.requestedSchema.required ?? []),
[request.requestedSchema.required],
);
const isValid = React.useMemo(() => {
return fields.every(([fieldName, fieldSchema]) => {
const value = formData[fieldName];
return validateField(value, fieldSchema, requiredSet.has(fieldName)).valid;
});
}, [fields, formData, requiredSet]);Reply with "@CharlieHelps yes please" if you'd like me to add a commit with this suggestion.
| const handleSingleEntryChange = React.useCallback( | ||
| (name: string, value: unknown) => { | ||
| const updatedData = { ...formData, [name]: value }; | ||
| setFormData(updatedData); | ||
| setTouchedFields((prev) => new Set(prev).add(name)); | ||
| onResponse({ action: "accept", content: updatedData }); | ||
| }, | ||
| [formData, onResponse], | ||
| ); |
There was a problem hiding this comment.
handleSingleEntryChange always responds with { action: "accept" } without checking validity/required constraints. If the single-entry schema is required and the user selects an option, that’s fine—but if defaults/clearing behaviors ever allow undefined or invalid values, this will submit invalid payloads. More importantly: it bypasses the same guard you apply in handleAccept, so the behavior differs by mode.
Given isSingleEntryMode only checks field type, not required, minimum, etc., it’s safer to validate before auto-accepting and to mark the field touched if invalid.
Suggestion
Validate the updated value before auto-accepting; if invalid, only update state + touched and do not call onResponse:
const handleSingleEntryChange = React.useCallback(
(name: string, value: unknown) => {
const updatedData = { ...formData, [name]: value };
setFormData(updatedData);
setTouchedFields((prev) => new Set(prev).add(name));
const schema = fields.find(([n]) => n === name)?.[1];
const required = requiredFields.includes(name);
if (schema && validateField(value, schema, required).valid) {
onResponse({ action: "accept", content: updatedData });
}
},
[fields, formData, onResponse, requiredFields],
);Reply with "@CharlieHelps yes please" if you'd like me to add a commit with this suggestion.
| const handleSingleEntryChange = React.useCallback( | ||
| (name: string, value: unknown) => { | ||
| const updatedData = { ...formData, [name]: value }; | ||
| setFormData(updatedData); | ||
| setTouchedFields((prev) => new Set(prev).add(name)); | ||
| onResponse({ action: "accept", content: updatedData }); | ||
| }, | ||
| [formData, onResponse], | ||
| ); |
There was a problem hiding this comment.
handleSingleEntryChange closes over formData and is re-created whenever formData changes. More importantly, it uses the captured formData value to build updatedData. Under rapid interactions / concurrent rendering, this pattern is easier to get wrong than using a functional state update.
Even if this is currently OK, it’s a sharp edge for a root component that’s intended as a reusable primitive.
Suggestion
Use a functional setFormData update to avoid stale closures and make updates atomic:
const handleSingleEntryChange = React.useCallback(
(name: string, value: unknown) => {
setTouchedFields((prev) => new Set(prev).add(name));
setFormData((prev) => {
const updated = { ...prev, [name]: value };
onResponse({ action: "accept", content: updated });
return updated;
});
},
[onResponse],
);(If calling onResponse inside the setter is undesirable, you can compute via a ref or queue a microtask.) Reply with "@CharlieHelps yes please" if you'd like me to add a commit with this suggestion.
| if ("pattern" in stringSchema && stringSchema.pattern) { | ||
| try { | ||
| const regex = new RegExp(stringSchema.pattern as string); | ||
| if (!regex.test(stringValue)) { | ||
| return { | ||
| valid: false, | ||
| error: "Value does not match required pattern", | ||
| }; | ||
| } | ||
| } catch { | ||
| // Invalid regex pattern, skip validation | ||
| } | ||
| } |
There was a problem hiding this comment.
The pattern branch uses new RegExp(stringSchema.pattern as string).
- The
as stringcast is unnecessary and weakens confidence around the runtime shape. - Swallowing regex compilation errors silently can lead to schemas that appear to “work” but never enforce pattern constraints—hard to debug.
Even if you choose to ignore invalid patterns, consider surfacing a deterministic validation error rather than skipping validation entirely.
Suggestion
Avoid the assertion and return a predictable error when the pattern itself is invalid:
if ("pattern" in stringSchema && stringSchema.pattern) {
try {
const regex = new RegExp(stringSchema.pattern);
if (!regex.test(stringValue)) {
return { valid: false, error: "Value does not match required pattern" };
}
} catch {
return { valid: false, error: "Invalid validation pattern" };
}
}Reply with "@CharlieHelps yes please" if you'd like me to add a commit with this suggestion.
| const StyledField: React.FC<{ | ||
| name: string; | ||
| schema: { type: string; enum?: unknown }; | ||
| autoFocus?: boolean; | ||
| }> = ({ name, schema, autoFocus }) => { | ||
| if (schema.type === "boolean") { | ||
| return <BooleanField {...props} />; | ||
| return <StyledBooleanField name={name} autoFocus={autoFocus} />; | ||
| } | ||
|
|
||
| if (schema.type === "string" && "enum" in schema) { | ||
| return <EnumField {...props} />; | ||
| return <StyledEnumField name={name} autoFocus={autoFocus} />; | ||
| } | ||
|
|
||
| if (schema.type === "string") { | ||
| return <StringField {...props} />; | ||
| return <StyledStringField name={name} autoFocus={autoFocus} />; | ||
| } | ||
|
|
||
| if (schema.type === "number" || schema.type === "integer") { | ||
| return <NumberField {...props} />; | ||
| return <StyledNumberField name={name} autoFocus={autoFocus} />; | ||
| } | ||
|
|
||
| return null; | ||
| }; |
There was a problem hiding this comment.
In StyledField, the schema prop is typed as { type: string; enum?: unknown } even though you’re passing the real schema from TamboElicitationRequest. This discards useful information and weakens local guarantees; it also relies on the 'enum' in schema check against a type that does not declare enumNames, format, etc.
Since this file already has access to TamboElicitationRequest, you can type the schema precisely and let the compiler help you keep the dispatcher aligned with the base components.
Suggestion
Type schema as the real property schema type:
type FieldSchema =
TamboElicitationRequest["requestedSchema"]["properties"][string];
const StyledField: React.FC<{
name: string;
schema: FieldSchema;
autoFocus?: boolean;
}> = ({ name, schema, autoFocus }) => { /* ... */ };Reply with "@CharlieHelps yes please" if you'd like me to add a commit with this suggestion.
| <button | ||
| type="button" | ||
| onClick={handleCancel} | ||
| data-slot="elicitation-ui-cancel-button" | ||
| > | ||
| Cancel | ||
| </button> | ||
| <button | ||
| type="button" | ||
| onClick={handleDecline} | ||
| data-slot="elicitation-ui-decline-button" | ||
| > | ||
| Decline | ||
| </button> | ||
| {!isSingleEntry && ( | ||
| <button | ||
| type="button" | ||
| onClick={handleAccept} | ||
| disabled={!isValid} | ||
| data-slot="elicitation-ui-submit-button" | ||
| > | ||
| Submit | ||
| </button> | ||
| )} |
There was a problem hiding this comment.
The base Actions component defaults the submit button to type="button". In multi-field mode, consumers may wrap this UI in a <form> and expect Enter-key submission / native form semantics. With type="button", you lose that by default.
Given this is a base primitive, defaults should lean toward correct semantics when possible.
Suggestion
Consider rendering the submit action as type="submit" by default (and keep cancel/decline as button). If you don’t want to enforce form semantics, at least document it clearly or provide a prop to opt into submit semantics.
Example:
<button
type="submit"
onClick={handleAccept}
disabled={!isValid}
data-slot="elicitation-ui-submit-button"
>
Submit
</button>Reply with "@CharlieHelps yes please" if you'd like me to add a commit with this suggestion.
3d02498 to
b753c1c
Compare
17463af to
a438f21
Compare
b753c1c to
c07cab3
Compare
Fixes TAM-1050 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ields Fix TypeScript type errors in elicitation-ui-field caused by spreading children render fn typed as ElicitationUIFieldRenderProps into sub-field components that expect narrower types.
… render prop in elicitation-ui
a438f21 to
a24846c
Compare
Summary
Adds
elicitation-uicompound component base primitives and styled wrapper.Base: Root, Title, Field, StringField, NumberField, BooleanField, EnumField, Actions + validation
Styled: Composes base components with Tailwind styling
Fixes TAM-1050
Test Plan
npm run lint && npm run check-types && npm test🤖 Generated with Claude Code