feat(react-ui-base): add scrollable-message-container compound component#2276
feat(react-ui-base): add scrollable-message-container compound component#2276lachieh 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 issue is architectural: ScrollableMessageContainerRoot lives in react-ui-base but directly depends on @tambo-ai/react thread state, making the “base primitive” not actually generic and likely to throw if used outside a Tambo provider. There are also performance/UX concerns around always using behavior: "smooth" for auto-scrolling (especially during streaming). Lastly, the context ref typing is inconsistent with how it’s mutated, and the styled wrapper’s props/asChild forwarding is redundant/surprising.
Additional notes (1)
- Readability |
...nents/scrollable-message-container/scrollable-message-container.tsx:11-11
The wrapper component’s props type intersectsScrollableMessageContainerViewportPropswithReact.HTMLAttributes<HTMLDivElement>. SinceScrollableMessageContainerViewportPropsis alreadyBaseProps<React.HTMLAttributes<HTMLDivElement>>, this intersection is redundant and can confuse consumers (and can lead to harder-to-read generated docs).
Also, in the wrapper, {...props} is spread onto Viewport while Root is rendered as asChild with no props passed—so any asChild passed to the wrapper will be forwarded to Viewport, not Root. If the wrapper is meant to preserve the original single-element API, it’s fine to not expose asChild at all, but currently it’s implicitly exposed and does something surprising.
Summary of changes
Added ScrollableMessageContainer compound component (react-ui-base)
- Exported a new package entrypoint
./scrollable-message-containerinpackages/react-ui-base/package.jsonto publish ESM/CJS builds (dist/esm/...,dist/cjs/...). - Re-exported
ScrollableMessageContainerand its related types frompackages/react-ui-base/src/index.ts. - Introduced a new compound component under
packages/react-ui-base/src/scrollable-message-container/:Rootprimitive that manages scroll state, auto-scroll behavior, and provides context.Viewportprimitive that binds the scrollable element to the root’sviewportRef.ScrollToBottomprimitive that exposes render props (isAtBottom,scrollToBottom).
Updated ui-registry to consume the base primitives
- Updated
packages/ui-registry/src/components/scrollable-message-container/config.jsonto depend on@tambo-ai/react-ui-base@nextand to include the base registry files. - Refactored the styled
ScrollableMessageContainerwrapper to composeScrollableMessageContainerBase.Root+Viewport, preserving the previous single-component API surface while moving behavior into the base package.
| import { Slot } from "@radix-ui/react-slot"; | ||
| import { GenerationStage, useTambo } from "@tambo-ai/react"; | ||
| import * as React from "react"; | ||
| import { BaseProps } from "../../types/component-render-or-children"; | ||
| import { ScrollableMessageContainerRootContext } from "./scrollable-message-container-root-context"; | ||
|
|
||
| export type ScrollableMessageContainerRootProps = BaseProps< | ||
| React.HTMLAttributes<HTMLDivElement> | ||
| >; | ||
|
|
||
| /** | ||
| * Root primitive for the scrollable message container component. | ||
| * Manages auto-scroll state, scroll position tracking, and thread message observation. | ||
| * Provides context for child components (Viewport, ScrollToBottom). | ||
| * @returns The root scrollable message container element with context provider | ||
| */ | ||
| export const ScrollableMessageContainerRoot = React.forwardRef< | ||
| HTMLDivElement, | ||
| ScrollableMessageContainerRootProps | ||
| >(function ScrollableMessageContainerRoot( | ||
| { children, asChild, ...props }, | ||
| ref, | ||
| ) { | ||
| const viewportRef = React.useRef<HTMLDivElement>(null); | ||
| const { thread } = useTambo(); | ||
| const [shouldAutoscroll, setShouldAutoscroll] = React.useState(true); | ||
| const [isAtBottom, setIsAtBottom] = React.useState(true); | ||
| const lastScrollTopRef = React.useRef(0); | ||
|
|
||
| const messagesContent = React.useMemo(() => { | ||
| if (!thread.messages) return null; | ||
|
|
||
| return thread.messages.map((message) => ({ | ||
| id: message.id, | ||
| content: message.content, | ||
| tool_calls: message.tool_calls, | ||
| component: message.component, | ||
| reasoning: message.reasoning, | ||
| componentState: message.componentState, | ||
| })); | ||
| }, [thread.messages]); | ||
|
|
||
| const generationStage = React.useMemo( | ||
| () => thread?.generationStage ?? GenerationStage.IDLE, | ||
| [thread?.generationStage], | ||
| ); |
There was a problem hiding this comment.
ScrollableMessageContainerRoot is part of react-ui-base, but it directly imports and depends on @tambo-ai/react (useTambo, GenerationStage). That makes this “base primitive” tightly coupled to the Tambo thread model and effectively not reusable outside that context.
This is a design/correctness issue for a base component library: consumers who want a generic scrollable container will be forced to install and run Tambo thread logic, and the component becomes harder to test in isolation. It also creates an implicit contract that the Root must be used inside a TamboProvider, otherwise useTambo() will likely throw at runtime.
If the intent is truly “message container for Tambo threads”, consider moving it to a Tambo-specific package (or to ui-registry only). If you want it in react-ui-base, the Root should accept the relevant signals as props (e.g., contentKey, isStreaming, onReachBottom, etc.) rather than reaching into Tambo state itself.
Suggestion
Decouple react-ui-base from @tambo-ai/react by lifting “what changed” and “streaming vs not” into props, and remove the useTambo() dependency.
For example:
export type ScrollableMessageContainerRootProps = BaseProps<
React.HTMLAttributes<HTMLDivElement> & {
/** Changes when content changes; used to trigger autoscroll */
contentKey?: unknown;
/** Whether content is currently streaming (affects scroll scheduling) */
isStreaming?: boolean;
}
>;Then use contentKey in the autoscroll effect deps instead of messagesContent, and use isStreaming instead of GenerationStage.
Reply with "@CharlieHelps yes please" if you'd like me to add a commit implementing this refactor (including updating the ui-registry wrapper to pass the appropriate values from useTambo()).
| const scrollToBottom = React.useCallback(() => { | ||
| if (!viewportRef.current) return; | ||
|
|
||
| viewportRef.current.scrollTo({ | ||
| top: viewportRef.current.scrollHeight, | ||
| behavior: "smooth", | ||
| }); | ||
| }, []); | ||
|
|
||
| const suspendAutoscroll = React.useCallback(() => { | ||
| setShouldAutoscroll(false); | ||
| }, []); | ||
|
|
||
| const resumeAutoscroll = React.useCallback(() => { | ||
| setShouldAutoscroll(true); | ||
| }, []); | ||
|
|
||
| /** | ||
| * Handle scroll events from the viewport to detect user scrolling direction | ||
| * and update isAtBottom state. | ||
| */ | ||
| const handleViewportScroll = React.useCallback(() => { | ||
| if (!viewportRef.current) return; | ||
|
|
||
| const { scrollTop, scrollHeight, clientHeight } = viewportRef.current; | ||
| const atBottom = Math.abs(scrollHeight - scrollTop - clientHeight) < 8; | ||
|
|
||
| setIsAtBottom(atBottom); | ||
|
|
||
| if (scrollTop < lastScrollTopRef.current) { | ||
| setShouldAutoscroll(false); | ||
| } else if (atBottom) { | ||
| setShouldAutoscroll(true); | ||
| } | ||
|
|
||
| lastScrollTopRef.current = scrollTop; | ||
| }, []); | ||
|
|
||
| /** | ||
| * Attach scroll listener to the viewport element. | ||
| */ | ||
| React.useEffect(() => { | ||
| const viewport = viewportRef.current; | ||
| if (!viewport) return; | ||
|
|
||
| viewport.addEventListener("scroll", handleViewportScroll); | ||
| return () => { | ||
| viewport.removeEventListener("scroll", handleViewportScroll); | ||
| }; | ||
| }, [handleViewportScroll]); | ||
|
|
||
| /** | ||
| * Auto-scroll to bottom when message content changes and autoscroll is enabled. | ||
| */ | ||
| React.useEffect(() => { | ||
| if (viewportRef.current && messagesContent && shouldAutoscroll) { | ||
| const scroll = () => { | ||
| if (viewportRef.current) { | ||
| viewportRef.current.scrollTo({ | ||
| top: viewportRef.current.scrollHeight, | ||
| behavior: "smooth", | ||
| }); | ||
| } | ||
| }; | ||
|
|
||
| if (generationStage === GenerationStage.STREAMING_RESPONSE) { | ||
| requestAnimationFrame(scroll); | ||
| } else { | ||
| const timeoutId = setTimeout(scroll, 50); | ||
| return () => clearTimeout(timeoutId); | ||
| } | ||
| } | ||
| }, [messagesContent, generationStage, shouldAutoscroll]); |
There was a problem hiding this comment.
scrollToBottom() always uses behavior: "smooth". When called during streaming updates and/or in the auto-scroll effect, this can cause jank (repeated smooth scroll animations) and can also fight user intent if they briefly scroll.
You already differentiate streaming vs non-streaming for scheduling; the same distinction should usually apply to scroll behavior (instant during streaming, smooth for user-initiated button clicks).
Suggestion
Add an option/parameter to scrollToBottom (or a separate scrollToBottomInstant) and use instant scrolling for auto-scroll while keeping smooth scrolling for explicit user actions.
Example:
const scrollToBottom = React.useCallback((opts?: { behavior?: ScrollBehavior }) => {
const el = viewportRef.current;
if (!el) return;
el.scrollTo({ top: el.scrollHeight, behavior: opts?.behavior ?? "auto" });
}, []);- In the auto-scroll effect:
scrollToBottom({ behavior: "auto" }) - In
ScrollToBottombutton:scrollToBottom({ behavior: "smooth" })
Reply with "@CharlieHelps yes please" if you'd like me to add a commit with this change.
| /** | ||
| * Handle scroll events from the viewport to detect user scrolling direction | ||
| * and update isAtBottom state. | ||
| */ | ||
| const handleViewportScroll = React.useCallback(() => { | ||
| if (!viewportRef.current) return; | ||
|
|
||
| const { scrollTop, scrollHeight, clientHeight } = viewportRef.current; | ||
| const atBottom = Math.abs(scrollHeight - scrollTop - clientHeight) < 8; | ||
|
|
||
| setIsAtBottom(atBottom); | ||
|
|
||
| if (scrollTop < lastScrollTopRef.current) { | ||
| setShouldAutoscroll(false); | ||
| } else if (atBottom) { | ||
| setShouldAutoscroll(true); | ||
| } | ||
|
|
||
| lastScrollTopRef.current = scrollTop; | ||
| }, []); |
There was a problem hiding this comment.
handleViewportScroll toggles shouldAutoscroll based on scroll direction using lastScrollTopRef. This can disable autoscroll even when the user scrolls only slightly (e.g., trackpad jitter) and doesn’t consider programmatic smooth scrolling (triggered by scrollToBottom) which also fires scroll events and can flip state unexpectedly.
It’s safer to drive autoscroll primarily from atBottom (with tolerance), and only suspend on explicit user intent (wheel/touch/scrollbar drag) or when atBottom becomes false.
Suggestion
Simplify the state machine:
- Compute
atBottomand always setisAtBottom. - Set
shouldAutoscrolltoatBottom(or only resume whenatBottomis true), and suspend only whenatBottombecomes false and the event was user-initiated.
A pragmatic improvement without complex pointer heuristics:
const atBottom = ...;
setIsAtBottom(atBottom);
setShouldAutoscroll(atBottom);If you need “suspend even when near-bottom but user scrolls up a bit”, add onWheel/onTouchMove handlers on the viewport to call suspendAutoscroll().
Reply with "@CharlieHelps yes please" if you'd like me to add a commit that adjusts the autoscroll logic and reduces unexpected toggling.
| export interface ScrollableMessageContainerRootContextValue { | ||
| /** Whether the container should auto-scroll to the bottom when content changes. */ | ||
| shouldAutoscroll: boolean; | ||
| /** Whether the scroll position is currently at the bottom of the container. */ | ||
| isAtBottom: boolean; | ||
| /** Scroll the container to the bottom. */ | ||
| scrollToBottom: () => void; | ||
| /** Suspend auto-scrolling (e.g., when user scrolls up). */ | ||
| suspendAutoscroll: () => void; | ||
| /** Resume auto-scrolling (e.g., when user scrolls to bottom). */ | ||
| resumeAutoscroll: () => void; | ||
| /** Ref for the scrollable viewport element. */ | ||
| viewportRef: React.RefObject<HTMLDivElement | null>; | ||
| } |
There was a problem hiding this comment.
In Viewport, the context ref is typed as RefObject<HTMLDivElement | null> but you’re mutating it by casting to MutableRefObject. This works because it’s actually created via useRef in the root, but the current context typing communicates “read-only” while the viewport relies on mutability.
This mismatch makes the API brittle: if the ref implementation ever changes, this becomes a footgun. It also encourages unsafe casts in downstream primitives.
Suggestion
Change the context type to accurately reflect intended usage by storing a React.MutableRefObject<HTMLDivElement | null>.
export interface ScrollableMessageContainerRootContextValue {
// ...
viewportRef: React.MutableRefObject<HTMLDivElement | null>;
}Then remove the cast in Viewport and assign directly.
Reply with "@CharlieHelps yes please" if you'd like me to add a commit updating the context typing + viewport assignment.
| 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.
The Viewport assigns the context ref by casting viewportRef to MutableRefObject and manually merges forwarded refs. This is brittle and easy to get wrong (especially if viewportRef type changes), and it’s a pattern that tends to get duplicated.
Prefer a small composeRefs/mergeRefs utility or useCallback ref that sets both without type assertions.
Suggestion
Replace the cast + manual branching with a composeRefs helper.
Example:
function composeRefs<T>(...refs: Array<React.Ref<T> | undefined>) {
return (node: T) => {
for (const ref of refs) {
if (!ref) continue;
if (typeof ref === "function") ref(node);
else (ref as React.MutableRefObject<T>).current = node;
}
};
}Then:
ref={composeRefs(ref, viewportRef)}This removes the unsafe assertion and simplifies the component.
Reply with "@CharlieHelps yes please" if you'd like me to add a commit with this ref composition change.
| const { content, componentProps } = useRender(props, { | ||
| isAtBottom, | ||
| scrollToBottom, | ||
| }); | ||
|
|
||
| return ( | ||
| <Comp | ||
| ref={ref} | ||
| data-slot="scrollable-message-container-scroll-to-bottom" | ||
| data-at-bottom={isAtBottom || undefined} | ||
| {...componentProps} | ||
| > | ||
| {content} | ||
| </Comp> | ||
| ); |
There was a problem hiding this comment.
ScrollableMessageContainerScrollToBottom always renders a wrapping div/Slot even when there is no children and no render function output. The docstring says it can return null, but the implementation doesn’t.
Rendering an empty element can affect layout, focus order, and CSS selectors (e.g. spacing utilities).
Suggestion
Return null when there’s nothing to render.
After useRender, do:
if (content == null) return null;(Use whatever “empty” semantics your useRender contract defines.)
Reply with "@CharlieHelps yes please" if you'd like me to add a commit updating this component to truly return null when there’s no content.
3d02498 to
b753c1c
Compare
5a5d2ad to
688fafc
Compare
b753c1c to
c07cab3
Compare
Fixes TAM-1061 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
688fafc to
4bdf243
Compare
Summary
Adds
scrollable-message-containercompound component base primitives and styled wrapper.Base: Root, Viewport, ScrollToBottom
Styled: Composes base components with Tailwind styling
Fixes TAM-1061
Test Plan
npm run lint && npm run check-types && npm test🤖 Generated with Claude Code