feat(react-ui-base): add MessageThreadPanel base primitive#2283
feat(react-ui-base): add MessageThreadPanel base primitive#2283lachieh wants to merge 17 commits intolachieh/tam-1057-message-suggestionsfrom
Conversation
- Create headless base compound components in packages/ui-registry/src/base/message-input/ - Refactor styled MessageInput to compose base components instead of duplicating logic - Move useCombinedResourceList and useCombinedPromptList hooks to base layer - Add styled-compound-wrappers skill with patterns for composing base components - Update compound-components skill with pre-computed props array pattern Key patterns: - Use data-* attributes for CSS styling, render props only for behavior changes - Pre-compute props arrays in useMemo for collections (not getter functions) - Icon factories allow styled layer to provide visual elements to base hooks Reduces styled MessageInput from ~1600 lines to ~1200 lines by eliminating duplicate context, hooks, state management, and event handlers.
- Refactored styled MessageInputTextarea and MessageInputPlainTextarea to use MessageInputBase.Textarea with render props instead of importing useMessageInputContext directly - Created MessageInputBase.ValueAccess component for MCP buttons to access value/setValue/editorRef via render props - Consolidated MAX_IMAGES and IS_PASTED_IMAGE constants to base context - Removed duplicate ResourceProvider/PromptProvider interfaces from styled layer (now re-exported from base) - Deleted unused DropZone and Elicitation base components - Added performance improvements: memoized render props objects and stable callbacks using functional state updates - Fixed type safety: inputRef now properly typed as TamboEditor | null Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- data-[dragging]: → data-dragging: - group-data-[dragging]: → group-data-dragging: - min-h-[82px] → min-h-20.5 - flex-shrink-0 → shrink-0 - bg-gradient-to-t → bg-linear-to-t - Remove verbose JSDoc typedef comment - Clean up base index exports (remove Context exposure)
…egistry - Move message-input base compound components from packages/ui-registry/src/base/ to packages/react-ui-base/src/message-input/ - Add message-input subpath export to react-ui-base package.json - Export MessageInput and all related types from react-ui-base main index - Update styled MessageInput to import from @tambo-ai/react-ui-base/message-input - Update skill documentation to reference new import path - Fix McpPromptEffectProps to accept null from useTamboMcpPrompt hook
Jest couldn't resolve @tambo-ai/react-ui-base/message and other subpath imports, breaking message component tests.
Internalizes debounce logic to avoid bundling use-debounce, which broke rollup's preserveModules output paths for subpath exports.
…mponents Refactors the following components to use compound component pattern: - canvas-space (TAM-1047) - edit-with-tambo-button (TAM-1049) - elicitation-ui (TAM-1050) - form (TAM-1051) - graph (TAM-1052) - input-fields (TAM-1053) - map (TAM-1054) - message-suggestions (TAM-1057) - scrollable-message-container (TAM-1061) - thread-content (TAM-1062) - thread-dropdown (TAM-1063) - thread-history (TAM-1064) Each component now has: - Base unstyled compound components in react-ui-base - Context providers with useRender/asChild support - Styled wrappers in ui-registry using the base components - Subpath exports for direct imports
- Mark `render` prop as @deprecated in types (kept for backwards compat) - Update useRender to prefer children-as-function over render prop - Convert message-suggestions styled wrapper from render= to children - Update compound-components and styled-compound-wrappers skills - Update plan doc with pattern change note and correct paths Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Fix no-nested-ternary lint errors in control-bar-root, edit-with-tambo-button-root, and input-fields-root by replacing nested ternaries with if/else - Fix TypeScript type errors in elicitation-ui-field by stripping children/render before dispatching to sub-field components
Adds the ./control-bar subpath export entry in package.json to match all other component subpath exports.
… render prop Convert all 18 render= prop usages across 10 styled wrapper files to children-as-function pattern, consistent with the deprecation of the render prop in favor of children-as-function.
- Fix render priority in control-bar, edit-with-tambo-button, and
input-fields root components to prefer children over render prop,
matching useRender hook behavior
- Use FieldSchema type instead of loose {type: string} in styled
elicitation-ui wrapper
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
Several base primitives currently include behavior that makes composition brittle: global side effects (ThreadHistoryRoot mutating documentElement), non-composed event handlers that can be overwritten by consumer props, and shortcut/key handling that may be inconsistent across platforms. There’s also a concrete config issue: duplicated moduleNameMapper entries in ui-registry Jest config. Addressing these will improve correctness, reusability, and alignment with the project’s headless primitive patterns.
Additional notes (1)
- Maintainability |
packages/ui-registry/jest.config.ts:22-35
packages/ui-registry/jest.config.tsnow includes two identicalmoduleNameMapperentries for^@tambo-ai/react-ui-base/(.*)$. This duplication is confusing and risks one of them being edited later while the other remains, causing brittle behavior.
Only one mapping should exist.
Summary of changes
What changed
New base primitives in @tambo-ai/react-ui-base
- Added new compound-component primitives for:
scrollable-message-container:ViewportandScrollToBottomprimitives.thread-content:Root,MessageList, andMessageprimitives, plusgetMessageKey.thread-dropdown:Root,Trigger,Menu,NewThreadItem, andThreadItemprimitives.thread-history:Rootand supporting primitives (Header,CollapseToggle,NewThreadButton,SearchInput,List,Item).
Render-prop API standardization
- Updated
PropsWithChildrenOrRenderFunctionto preferchildrenas function and markrenderas deprecated. - Updated
useRender()to prefer children-as-function over deprecatedrenderprop.
ui-registry refactors to compose base primitives
- Refactored multiple styled components in
packages/ui-registry/src/components/*to compose the new base primitives rather than duplicating context/state logic (e.g.CanvasSpace,EditWithTamboButton,ElicitationUI,Form,Graph,InputFields,Map,MessageInput,MessageSuggestions,ScrollableMessageContainer,ThreadContent,ThreadDropdown,ThreadHistory).
Tooling/test config updates
- Updated
packages/ui-registry/jest.config.tsto map@tambo-ai/react-ui-base/*subpath imports to source.
Docs / planning
- Added
plans/compound-component-refactor.mddocumenting phases and patterns.
| const { viewportRef } = useScrollableMessageContainerRootContext(); | ||
|
|
||
| const Comp = asChild ? Slot : "div"; | ||
|
|
||
| return ( | ||
| <Comp | ||
| ref={(node: HTMLDivElement | null) => { | ||
| // Assign to the context ref | ||
| (viewportRef as React.MutableRefObject<HTMLDivElement | null>).current = | ||
| node; | ||
|
|
||
| // Forward the external ref | ||
| if (typeof ref === "function") { | ||
| ref(node); | ||
| } else if (ref) { | ||
| ref.current = node; | ||
| } | ||
| }} |
There was a problem hiding this comment.
viewportRef is being assigned via a forced cast to MutableRefObject and a bespoke inline ref-forwarding implementation. This is brittle and easy to get subtly wrong (especially if viewportRef is already a callback ref or not guaranteed to be mutable), and it duplicates a common pattern used elsewhere.
This also makes it harder to maintain and audit for correct ref behavior with asChild (Slot) usage.
Suggestion
Consider using Radix's composeRefs (@radix-ui/react-compose-refs) or a small internal mergeRefs helper to merge viewportRef and the forwarded ref without as casting.
Example:
import { composeRefs } from "@radix-ui/react-compose-refs";
...
const composedRef = React.useMemo(
() => composeRefs(viewportRef, ref),
[viewportRef, ref],
);
return (
<Comp
ref={composedRef}
data-slot="scrollable-message-container-viewport"
{...props}
>
{children}
</Comp>
);This removes the unsafe cast and centralizes correct ref semantics. Reply with "@CharlieHelps yes please" if you'd like me to add a commit with this suggestion.
| React.useEffect(() => { | ||
| const handleKeyDown = (event: KeyboardEvent) => { | ||
| if (event.altKey && event.shiftKey && event.key === "n") { | ||
| event.preventDefault(); | ||
| void handleNewThread(); | ||
| } | ||
| }; | ||
|
|
||
| document.addEventListener("keydown", handleKeyDown); | ||
|
|
||
| return () => { | ||
| document.removeEventListener("keydown", handleKeyDown); | ||
| }; | ||
| }, [handleNewThread]); |
There was a problem hiding this comment.
The keyboard shortcut handler uses event.key === "n". On some keyboards/layouts and with Shift held, event.key can be uppercase ("N") or vary; additionally, the code is hard-coded to Alt+Shift+N even though the UI advertises a platform-specific modifier.
This can lead to shortcuts that work inconsistently depending on layout and OS, and it also ignores the isMac/modKey intention (currently modKey is a symbol on Mac but the listener still checks altKey).
Suggestion
Normalize the key and align the actual shortcut logic with the label you present:
- Use
event.key.toLowerCase() === "n". - Decide whether Mac should be
alt+shift+normeta+shift+n(and ensure both label + handler match).
Example:
const isTrigger = isMac
? event.metaKey && event.shiftKey && event.key.toLowerCase() === "n"
: event.altKey && event.shiftKey && event.key.toLowerCase() === "n";Or keep Alt everywhere but make labels match (modKey should then be "Alt"/"⌥"). Reply with "@CharlieHelps yes please" if you'd like me to add a commit with this suggestion.
| React.useEffect(() => { | ||
| const handleKeyDown = (event: KeyboardEvent) => { | ||
| if (event.altKey && event.shiftKey && event.key === "n") { | ||
| event.preventDefault(); | ||
| void handleNewThread(); | ||
| } | ||
| }; | ||
|
|
||
| document.addEventListener("keydown", handleKeyDown); | ||
|
|
||
| return () => { | ||
| document.removeEventListener("keydown", handleKeyDown); | ||
| }; | ||
| }, [handleNewThread]); | ||
|
|
There was a problem hiding this comment.
ThreadDropdownRoot registers a global document.addEventListener("keydown", ...) for Alt+Shift+N. When multiple dropdown roots are mounted (common in component libraries, storybooks, or multi-panel UIs), this will create duplicated handlers and multiple thread creations per keypress.
Also, this listener fires even when focus is inside text inputs/textarea, which is likely undesirable.
Suggestion
Guard the shortcut and avoid global duplication. Options:
- Only enable the listener when the dropdown is open / focused, or when a prop enables shortcuts.
- Ignore key presses originating from editable elements.
- Use an
AbortControllerto ensure cleanup and simplify effect logic.
Example guard:
const isEditableTarget = (t: EventTarget | null) => {
const el = t as HTMLElement | null;
return !!el && (
el.tagName === "INPUT" ||
el.tagName === "TEXTAREA" ||
el.isContentEditable
);
};
React.useEffect(() => {
const onKeyDown = (event: KeyboardEvent) => {
if (isEditableTarget(event.target)) return;
if (event.altKey && event.shiftKey && event.key.toLowerCase() === "n") {
event.preventDefault();
void handleNewThread();
}
};
document.addEventListener("keydown", onKeyDown);
return () => document.removeEventListener("keydown", onKeyDown);
}, [handleNewThread]);Reply with "@CharlieHelps yes please" if you'd like me to add a commit with this change.
| // Update CSS variable when sidebar collapses/expands | ||
| React.useEffect(() => { | ||
| const sidebarWidth = isCollapsed ? "3rem" : "16rem"; | ||
| document.documentElement.style.setProperty("--sidebar-width", sidebarWidth); | ||
| }, [isCollapsed]); |
There was a problem hiding this comment.
This component mutates a global CSS variable on document.documentElement inside the base primitive. That’s a significant side effect for a headless primitive:
- It creates cross-instance coupling (multiple sidebars would race each other).
- It leaks styling decisions into base logic, reducing reusability.
- It can cause surprising global layout changes when this component mounts.
This is especially problematic since the repo’s own patterns emphasize unstyled/base primitives with styling controlled at the wrapper layer via data-* attributes.
Suggestion
Move the CSS variable mutation out of the base primitive and into the styled wrapper (or expose state via data-collapsed and let CSS handle width without JS). If you must support CSS-variable-based layouts, consider:
- Accepting a callback prop like
onSidebarWidthChange(width: string)in the base root, or - Applying the CSS variable on the root element only (not
documentElement), e.g.style={{ "--sidebar-width": ... } as React.CSSProperties }.
This keeps the primitive composable and avoids global side effects. Reply with "@CharlieHelps yes please" if you'd like me to add a commit with this suggestion.
| const expandOnSearch = React.useCallback(() => { | ||
| if (isCollapsed) { | ||
| setIsCollapsed(false); | ||
| setTimeout(() => { | ||
| searchInputRef.current?.focus(); | ||
| }, 300); | ||
| } | ||
| }, [isCollapsed, setIsCollapsed]); |
There was a problem hiding this comment.
setTimeout(..., 300) is used to focus the search input after expanding. This is timing-coupled to the CSS animation duration and can break if durations change, user prefers reduced motion, or the browser throttles timers.
Since this is a base primitive, it’s better to make focusing deterministic and not depend on magic numbers.
Suggestion
Prefer a deterministic focus strategy:
- Use
requestAnimationFrame(possibly twice) aftersetIsCollapsed(false), or - Focus on next layout effect when
isCollapsedbecomes false and ashouldFocusflag is set.
Example:
const [shouldFocus, setShouldFocus] = React.useState(false);
const expandOnSearch = React.useCallback(() => {
if (isCollapsed) {
setShouldFocus(true);
setIsCollapsed(false);
}
}, [isCollapsed, setIsCollapsed]);
React.useLayoutEffect(() => {
if (!isCollapsed && shouldFocus) {
searchInputRef.current?.focus();
setShouldFocus(false);
}
}, [isCollapsed, shouldFocus]);Reply with "@CharlieHelps yes please" if you'd like me to add a commit with this suggestion.
| return ( | ||
| <Comp | ||
| ref={ref} | ||
| data-slot="thread-history-collapse-toggle" | ||
| data-collapsed={isCollapsed || undefined} | ||
| data-position={position} | ||
| onClick={handleToggle} | ||
| aria-label={isCollapsed ? "Expand sidebar" : "Collapse sidebar"} | ||
| {...props} | ||
| > |
There was a problem hiding this comment.
ThreadHistoryCollapseToggle always sets onClick={handleToggle} and then spreads {...props} afterwards. If a consumer passes their own onClick, it will override the internal toggle behavior and the component will silently stop working.
For primitives, the internal behavior should be preserved and consumer handlers should be composed (called in addition).
Suggestion
Compose the click handler instead of letting it be overwritten:
const { onClick, ...rest } = props;
<Comp
...
onClick={(e) => {
onClick?.(e);
if (!e.defaultPrevented) handleToggle();
}}
{...rest}
/>This preserves the toggle while still allowing consumers to hook into the event. Reply with "@CharlieHelps yes please" if you'd like me to add a commit with this suggestion.
| return ( | ||
| <Comp | ||
| ref={ref} | ||
| data-slot="thread-history-item" | ||
| data-active={isActive || undefined} | ||
| data-thread-id={thread.id} | ||
| onClick={handleClick} | ||
| {...props} | ||
| > |
There was a problem hiding this comment.
Similar to the collapse toggle: ThreadHistoryItem sets onClick={handleClick} and then spreads {...props} after it, allowing consumer onClick to override the internal switch-thread behavior.
This is a correctness issue: the component may render but no longer switches threads when clicked.
Suggestion
Compose onClick like:
const { onClick, ...rest } = props;
<Comp
...
onClick={(e) => {
onClick?.(e);
if (!e.defaultPrevented) void handleClick();
}}
{...rest}
/>Also consider using a <button> by default for accessibility if this is intended to be interactive (or require role="button" + keyboard handlers). Reply with "@CharlieHelps yes please" if you'd like me to add a commit with this suggestion.
| return ( | ||
| <Comp | ||
| ref={ref} | ||
| type="button" | ||
| data-slot="thread-dropdown-trigger" | ||
| {...props} | ||
| > | ||
| {children} |
There was a problem hiding this comment.
ThreadDropdownTrigger forces type="button" unconditionally. When asChild is used, consumers may provide a non-<button> element (e.g. <a>), and type will be passed to it.
While React will generally ignore unknown attributes for many elements, this makes asChild composition leaky. The common Radix/Slot pattern is to only set type when rendering an actual <button>.
Suggestion
Only set type="button" when Comp is a real button:
<Comp
ref={ref}
{...(!asChild ? { type: "button" } : {})}
data-slot="thread-dropdown-trigger"
{...props}
>This keeps asChild usage cleaner. Reply with "@CharlieHelps yes please" if you'd like me to add a commit with this suggestion.
| export type PropsWithChildrenOrRenderFunction< | ||
| ComponentProps, | ||
| RenderPropProps = never, | ||
| > = | ||
| | React.PropsWithChildren<ComponentProps> | ||
| | PropsWithRenderFunction<ComponentProps, RenderPropProps>; | ||
| > = Omit<ComponentProps, "children"> & { | ||
| children?: React.ReactNode | ((props: RenderPropProps) => React.ReactNode); | ||
| /** @deprecated Use children as a function instead. */ | ||
| render?: ComponentRenderFn<RenderPropProps>; | ||
| }; |
There was a problem hiding this comment.
useRender() now prefers children-as-function, which is good. However, PropsWithChildrenOrRenderFunction is defined as Omit<ComponentProps, "children"> & { children?: ...; render?: ... }.
That means if a component's ComponentProps legitimately includes a children prop with a stricter type (e.g., children: (x) => ... only), you lose that type information. It’s a subtle but real DX regression for base primitives where children types matter.
Given the plan doc explicitly says render prop is deprecated and new code should use children-as-function, it’d be better if the helper preserved/parameterized the children type rather than erasing it.
Suggestion
Consider making PropsWithChildrenOrRenderFunction accept a Children generic (defaulting to React.ReactNode | ((props) => React.ReactNode)) rather than always omitting/overriding children.
Sketch:
export type PropsWithChildrenOrRenderFunction<
ComponentProps,
RenderPropProps = never,
Children = React.ReactNode | ((props: RenderPropProps) => React.ReactNode)
> = Omit<ComponentProps, "children"> & {
children?: Children;
/** @deprecated Use children as a function instead. */
render?: ComponentRenderFn<RenderPropProps>;
};Reply with "@CharlieHelps yes please" if you'd like me to add a commit with this change.
…prop refactor - Add MessageThreadCollapsible, MessageThreadFull, MessageThreadPanel base primitives - Add shared utils: useCanvasDetection, useMergeRefs, getPositioning - Replace className string parsing with explicit position prop across all layers - Update styled wrappers and showcase to use position prop Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
b753c1c to
c07cab3
Compare
Summary
Test plan
npm run check-types -w packages/react-ui-basepassesnpm run lint -w packages/react-ui-basepasses@tambo-ai/react-ui-base/message-thread-panelFixes TAM-1060
🤖 Generated with Claude Code