feat(react-ui-base): add mcp-components compound component primitives#2252
feat(react-ui-base): add mcp-components compound component primitives#2252
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
The new compound primitives are a solid direction, but a few base-level behaviors are too opinionated or incorrect. Most notably, McpResourceButtonSearchInput renders children into an <input> (invalid HTML) and both roots return null during loading/empty states, limiting composition and causing layout shift. Also, McpPromptButtonRoot likely triggers avoidable fetches by calling the prompt hook with an empty string when nothing is selected. Tightening these areas will make the primitives safer and more reusable.
Additional notes (2)
- Readability |
packages/react-ui-base/src/mcp-components/prompt-button/utils.ts:8-42
extractPromptTextis typed to returnstringbut the caller treats empty string as an error (if (!promptText)). The function currently returns""when nothing matches, which is fine, but the JSDoc says “Extracted text content joined by newlines” and doesn’t reflect the empty-result behavior.
More importantly, isValidPromptData only validates that messages is an array; extractPromptText then assumes message shapes. That’s ok for “best effort”, but the naming implies stronger validation than is actually performed.
- Readability |
packages/ui-registry/src/components/mcp-components/mcp-components.tsx:42-50
In the registry wrapper,McpPromptButtonBase.Rootis rendered withasChildbut wraps its child in an extra<div>.
Because Root ultimately renders a <div> by default (or merges props into the child with Slot), this extra wrapper can interfere with styling hooks, layout, and data-slot targeting (you likely want the data-slot="mcp-prompt-button" to land on the same element your CSS expects). Same pattern is used for McpResourceButtonBase.Root.
Summary of changes
What changed
@tambo-ai/react-ui-base
- Added a new subpath export
@tambo-ai/react-ui-base/mcp-componentsinpackage.jsonexports. - Re-exported MCP primitives from the root
src/index.ts(both values and types). - Introduced a new
src/mcp-components/*module implementing unstyled compound component primitives:McpPromptButton.{Root,Trigger,Menu,List,Item}McpResourceButton.{Root,Trigger,Menu,List,Item,SearchInput}
- Implemented Context + Root + sub-components pattern with:
useMcpPromptButtonContext/useOptionalMcpPromptButtonContextuseMcpResourceButtonContext/useOptionalMcpResourceButtonContext
- Added prompt parsing helpers:
extractPromptText,isValidPromptData.
@tambo-ai/ui-registry
- Updated
mcp-componentsregistry config to depend on@tambo-ai/react-ui-base. - Refactored the registry
mcp-components.tsxwrapper to use the new base primitives and contexts, removing duplicated prompt parsing and MCP hook logic.
| const { data: promptList, isLoading } = useTamboMcpPromptList(); | ||
| const [selectedPromptName, setSelectedPromptName] = React.useState< | ||
| string | null | ||
| >(null); | ||
| const [promptError, setPromptError] = React.useState<string | null>(null); | ||
| const { data: promptData, error: fetchError } = useTamboMcpPrompt( | ||
| selectedPromptName ?? "", | ||
| ); | ||
|
|
There was a problem hiding this comment.
McpPromptButtonRoot calls useTamboMcpPrompt(selectedPromptName ?? "") on every render. If the underlying hook treats an empty string as a real key, this can cause unnecessary fetches or error states whenever nothing is selected.
Even if the hook internally guards, this is an avoidable footgun in a base primitive that will be widely reused.
Suggestion
Consider gating the prompt fetch so it only runs when a valid selection exists. Depending on the @tambo-ai/react/mcp API, that might mean passing an enabled/skip option or lifting the hook behind a conditional wrapper if supported.
Example (if options exist):
const shouldFetch = selectedPromptName != null;
const { data: promptData, error: fetchError } = useTamboMcpPrompt(selectedPromptName ?? "", {
enabled: shouldFetch,
});If no options exist, introduce a small helper hook in this module that internally no-ops when selectedPromptName is null, rather than calling the network hook with "".
Reply with "@CharlieHelps yes please" if you'd like me to add a commit with this suggestion.
| // Only show if prompts are available (hide during loading and when no prompts) | ||
| if (!promptList || promptList.length === 0) { | ||
| return null; | ||
| } |
There was a problem hiding this comment.
The root returns null when promptList is undefined (initial load) or empty. That means the entire compound component disappears during loading, so consumers can’t show a disabled trigger/loading state, and layout may shift.
The comment says “hide during loading”, but this is a base primitive; hiding should be a consumer choice. Also, you’re computing/maintaining selection+error state and effects even though the UI is unmounted, which is awkward for composition.
Suggestion
Don’t hard-return null on !promptList. Prefer always rendering the provider/container and expose isLoading/isEmpty via context so the wrapper decides what to display.
For example:
const hasPrompts = !!promptList && promptList.length > 0;
// ...contextValue includes hasPrompts
return (
<Provider value={{...contextValue, hasPrompts}}>
<Comp ...>{children}</Comp>
</Provider>
);If you still want the convenience behavior, consider an explicit prop like hideWhenEmpty?: boolean defaulting to false.
Reply with "@CharlieHelps yes please" if you'd like me to add a commit with this suggestion.
| const Comp = asChild ? Slot : "input"; | ||
|
|
||
| return ( | ||
| <Comp | ||
| ref={ref} | ||
| type="text" | ||
| data-slot="mcp-resource-button-search-input" | ||
| value={searchQuery} | ||
| onChange={(e: React.ChangeEvent<HTMLInputElement>) => | ||
| setSearchQuery(e.target.value) | ||
| } | ||
| onClick={(e: React.MouseEvent) => e.stopPropagation()} | ||
| onKeyDown={(e: React.KeyboardEvent) => { | ||
| e.stopPropagation(); | ||
| if (e.key === "Escape") { | ||
| setIsOpen(false); | ||
| } | ||
| }} | ||
| {...rest} | ||
| > | ||
| {content} | ||
| </Comp> | ||
| ); |
There was a problem hiding this comment.
McpResourceButtonSearchInput renders an <input> by default, but still renders {content} as children of the input. Inputs are void elements; children are invalid HTML and will be ignored (and can trigger hydration/console warnings in React).
This is a correctness bug in the base primitive.
Suggestion
Do not render children for the default <input> case. If you need to support children for asChild, branch on asChild:
const Comp = asChild ? Slot : "input";
if (!asChild) {
return (
<input
ref={ref}
type="text"
data-slot="mcp-resource-button-search-input"
value={searchQuery}
onChange={(e) => setSearchQuery(e.target.value)}
onClick={(e) => e.stopPropagation()}
onKeyDown={(e) => {
e.stopPropagation();
if (e.key === "Escape") setIsOpen(false);
}}
{...rest}
/>
);
}
return (
<Slot ref={ref} data-slot="mcp-resource-button-search-input" {...rest}>
{content}
</Slot>
);(And consider whether children should be allowed at all for this component if it’s primarily an input.)
Reply with "@CharlieHelps yes please" if you'd like me to add a commit with this suggestion.
| // Only show if resources are available | ||
| if (!resourceList || resourceList.length === 0) { | ||
| return null; | ||
| } |
There was a problem hiding this comment.
Same issue as the prompt root: the resource root returns null when resourceList is undefined (loading) or empty. This prevents consumers from showing a disabled/loading trigger and can cause layout shift.
Additionally, isOpen state is owned by the root. If the root unmounts due to resourceList becoming empty, open-state resets abruptly; that may be surprising in edge cases (e.g., list briefly undefined during refetch).
Suggestion
Avoid returning null from the base root. Always render the provider/container and expose resourceList, isLoading, and a derived isEmpty/hasResources flag.
If you want the old behavior, make it opt-in via a prop (e.g. hideWhenEmpty).
Reply with "@CharlieHelps yes please" if you'd like me to add a commit with this suggestion.
| const handleClick = () => { | ||
| if (onSelect) { | ||
| onSelect(); | ||
| } else { | ||
| onSelectPrompt(prompt.prompt.name); | ||
| } | ||
| }; | ||
|
|
||
| const Comp = asChild ? Slot : "div"; | ||
| const content = | ||
| typeof children === "function" ? children(renderProps) : children; | ||
|
|
||
| return ( | ||
| <Comp | ||
| ref={ref} | ||
| role="menuitem" | ||
| tabIndex={0} | ||
| data-slot="mcp-prompt-button-item" | ||
| onClick={handleClick} | ||
| onKeyDown={(e: React.KeyboardEvent) => { | ||
| if (e.key === "Enter" || e.key === " ") { | ||
| e.preventDefault(); | ||
| handleClick(); | ||
| } | ||
| }} |
There was a problem hiding this comment.
handleClick can fire twice when asChild is used and the child also has its own onClick, because Radix Slot merges event handlers (both run). That can lead to double insertion / double selection.
Also, onKeyDown triggers handleClick() on Enter/Space but does not guard against repeated keydown events (key repeat) which can also result in duplicate selection.
For compound primitives, it's safer to compose with user handlers and allow preventing the default selection behavior.
Suggestion
Support composed handlers and allow opting out via event.defaultPrevented, e.g.:
const handleSelect = () => {
if (onSelect) return onSelect();
onSelectPrompt(prompt.prompt.name);
};
onClick={(e) => {
props.onClick?.(e);
if (!e.defaultPrevented) handleSelect();
}}
onKeyDown={(e) => {
props.onKeyDown?.(e);
if (e.defaultPrevented) return;
if ((e.key === "Enter" || e.key === " ") && !e.repeat) {
e.preventDefault();
handleSelect();
}
}}(Same applies to the resource item.)
Reply with "@CharlieHelps yes please" if you'd like me to add a commit with this suggestion.
| const handleClick = () => { | ||
| if (onSelect) { | ||
| onSelect(); | ||
| } else { | ||
| onSelectResource( | ||
| resource.resource.uri, | ||
| resource.resource.name ?? resource.resource.uri, | ||
| ); | ||
| } | ||
| }; | ||
|
|
||
| const Comp = asChild ? Slot : "div"; | ||
| const content = | ||
| typeof children === "function" ? children(renderProps) : children; | ||
|
|
||
| return ( | ||
| <Comp | ||
| ref={ref} | ||
| role="menuitem" | ||
| tabIndex={0} | ||
| data-slot="mcp-resource-button-item" | ||
| onClick={handleClick} | ||
| onKeyDown={(e: React.KeyboardEvent) => { | ||
| if (e.key === "Enter" || e.key === " ") { | ||
| e.preventDefault(); | ||
| handleClick(); | ||
| } | ||
| }} | ||
| {...props} |
There was a problem hiding this comment.
Same issue as the prompt item: when asChild is used, merged handlers can cause the selection behavior to run in addition to consumer-provided onClick/onKeyDown, potentially resulting in double inserts.
Additionally, Space key handling on a div role="menuitem" can be inconsistent across screen readers unless you carefully emulate button behavior; guarding e.repeat helps prevent duplicate inserts.
Suggestion
Compose handlers and honor event.defaultPrevented (and !e.repeat) similar to:
const handleSelect = () => {
if (onSelect) return onSelect();
onSelectResource(resource.resource.uri, resource.resource.name ?? resource.resource.uri);
};
onClick={(e) => {
props.onClick?.(e);
if (!e.defaultPrevented) handleSelect();
}}
onKeyDown={(e) => {
props.onKeyDown?.(e);
if (e.defaultPrevented) return;
if ((e.key === "Enter" || e.key === " ") && !e.repeat) {
e.preventDefault();
handleSelect();
}
}}Reply with "@CharlieHelps yes please" if you'd like me to add a commit with this suggestion.
Add unstyled compound component base primitives for MCP prompt and resource buttons with Context + Root + sub-components pattern. New components in @tambo-ai/react-ui-base: - McpPromptButton.Root, .Trigger, .Menu, .List, .Item - McpResourceButton.Root, .Trigger, .Menu, .List, .Item, .SearchInput Features: - Full context system with useMcpPromptButtonContext/useMcpResourceButtonContext - Render function support for flexible customization - asChild prop support via @radix-ui/react-slot - data-slot attributes for CSS styling hooks - Subpath export: @tambo-ai/react-ui-base/mcp-components TAM-1055 Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
9e611a9 to
88abd9e
Compare
Summary
Changes
New Base Components
McpPromptButton.Root,.Trigger,.Menu,.List,.ItemMcpResourceButton.Root,.Trigger,.Menu,.List,.Item,.SearchInputFeatures
useMcpPromptButtonContext/useMcpResourceButtonContextasChildprop support via@radix-ui/react-slotdata-slotattributes for CSS styling hooks@tambo-ai/react-ui-base/mcp-componentsTest plan
Fixes TAM-1055
🤖 Generated with Claude Code