feat(react-ui-base): add thread-history compound component#2268
feat(react-ui-base): add thread-history compound component#2268lachieh wants to merge 3 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 new base primitives are generally well-structured, but several components (CollapseToggle, NewThreadButton, Item) allow consumer onClick props to override core internal behavior due to {...props} spread order, which is a real correctness/compatibility risk. ThreadHistoryRoot introduces a global CSS variable side effect without cleanup, which can leak state across mounts. The styled wrapper’s collapsed behavior likely changed subtly because it no longer applies the prior collapse transition classes and instead returns null in render, potentially affecting layout/scroll behavior.
Additional notes (1)
- Readability |
packages/ui-registry/src/components/thread-history/thread-history.tsx:271-289
In the styled wrapper,ThreadHistoryBase.Listis given aclassNamebut the render function returnsnullwhen collapsed. This is fine, but note that the baseListstill setsdata-*attributes and can be styled via[data-collapsed]. The wrapper also removed the previous collapsed transition classes; now the parent will always haveoverflow-y-auto flex-1 ...even when collapsed.
This can cause layout/scroll affordances to differ from the old behavior (e.g., still reserving space / scroll container present).
Summary of changes
What changed
✨ New ThreadHistory compound component in react-ui-base
- Added a new public export path
@tambo-ai/react-ui-base/thread-historyviapackages/react-ui-base/package.jsonexports. - Exposed
ThreadHistorynamespace and related prop/render-prop types frompackages/react-ui-base/src/index.ts. - Introduced base primitives under
packages/react-ui-base/src/thread-history/*:Root+ context (ThreadHistoryRootContext)Header,List,ItemCollapseToggle,NewThreadButton,SearchInput
- Implemented shared state in
ThreadHistoryRootusinguseTamboThreadList()/useTamboThread()and provided it via context.
🎨 Refactor UI registry component to compose base primitives
- Updated
packages/ui-registry/src/components/thread-history/thread-history.tsxto:- Remove the local context + thread filtering logic
- Compose UI using
ThreadHistoryBase.*components and render props - Keep styling and thread rename/generate interactions in the styled wrapper
| 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 hard-sets onClick={handleToggle} and then spreads {...props} afterwards. That means any consumer-provided onClick overrides the internal toggle, potentially breaking the component silently.
This is a correctness issue for a base primitive: it should always toggle, while still allowing consumers to run their own handler.
Suggestion
Change the click handler to compose with props.onClick instead of being overwritten (or overriding the consumer). For example:
const handleClick: React.MouseEventHandler<HTMLButtonElement> = (e) => {
props.onClick?.(e);
if (e.defaultPrevented) return;
setIsCollapsed((prev) => !prev);
};
return (
<Comp
...
onClick={handleClick}
{...props}
/>
);This keeps the primitive reliable while preserving extensibility. 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-new-thread-button" | ||
| data-collapsed={isCollapsed || undefined} | ||
| onClick={handleNewThread} | ||
| title="New thread" | ||
| {...props} | ||
| > |
There was a problem hiding this comment.
ThreadHistoryNewThreadButton has the same handler precedence issue: onClick={handleNewThread} is set but then {...props} comes after, allowing a consumer onClick to override and prevent new thread creation.
Also, handleNewThread stops propagation only when invoked with a mouse event. If consumers call it via their own click handler (or if composed later), behavior may differ.
Suggestion
Compose props.onClick with the internal action and ensure internal behavior still runs unless prevented:
const handleClick: React.MouseEventHandler<HTMLButtonElement> = async (e) => {
props.onClick?.(e);
if (e.defaultPrevented) return;
e.stopPropagation();
await handleNewThread();
};
return (
<Comp
...
onClick={handleClick}
{...props}
/>
);(You can inline handleNewThread and remove the optional event param.) 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-new-thread-button" | ||
| data-collapsed={isCollapsed || undefined} | ||
| onClick={handleNewThread} | ||
| title="New thread" | ||
| {...props} | ||
| > |
There was a problem hiding this comment.
ThreadHistoryNewThreadButton also allows onClick to override the internal handler
You pass onClick={handleNewThread} and then spread {...props} after, which means consumer onClick will override and prevent new thread creation.
Additionally, the global document.addEventListener('keydown', ...) means every mounted instance registers a hotkey and it will fire even when the user is typing in an input/textarea or when focus is inside a menu. That’s likely to cause surprising behavior.
Suggestion
-
Compose
onClicklike in the other primitives. -
Gate the keyboard shortcut to avoid triggering while typing, and consider requiring focus within the ThreadHistory root.
React.useEffect(() => {
const handleKeyDown = (event: KeyboardEvent) => {
const el = event.target as HTMLElement | null;
const isTypingTarget =
el?.tagName === "INPUT" ||
el?.tagName === "TEXTAREA" ||
(el as HTMLElement | null)?.isContentEditable;
if (isTypingTarget) return;
if (event.altKey && event.shiftKey && event.key.toLowerCase() === "n") {
event.preventDefault();
void handleNewThread();
}
};
document.addEventListener("keydown", handleKeyDown);
return () => document.removeEventListener("keydown", handleKeyDown);
}, [handleNewThread]);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.
ThreadHistoryItem also risks breaking its core behavior because {...props} comes after onClick={handleClick}. A consumer-provided onClick would replace the internal switchCurrentThread call.
Additionally, if you do compose handlers, make sure to pass the click event through so consumers can preventDefault() / stopPropagation() when needed.
Suggestion
Compose the click handler instead of allowing it to be overridden:
const handleItemClick: React.MouseEventHandler<HTMLDivElement> = (e) => {
props.onClick?.(e);
if (e.defaultPrevented) return;
switchCurrentThread(thread.id);
onThreadChange?.();
};
return (
<Comp
...
onClick={handleItemClick}
{...props}
/>
);Also consider using a semantic element (e.g. button) when not asChild so the item is keyboard accessible by default. Reply with "@CharlieHelps yes please" if you'd like me to add a commit with this suggestion.
| // 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.
ThreadHistoryRoot mutates a global CSS variable on document.documentElement but never restores the previous value on unmount. If multiple sidebars mount/unmount (or different pages set the same variable), this can leave stale global state behind.
Since react-ui-base is a primitives package, global side effects should be carefully scoped and cleaned up.
Suggestion
Capture the previous value and restore it in the effect cleanup:
React.useEffect(() => {
const el = document.documentElement;
const prev = el.style.getPropertyValue("--sidebar-width");
el.style.setProperty("--sidebar-width", isCollapsed ? "3rem" : "16rem");
return () => {
if (prev) el.style.setProperty("--sidebar-width", prev);
else el.style.removeProperty("--sidebar-width");
};
}, [isCollapsed]);Alternatively, consider moving this responsibility to the styled wrapper (ui-registry) rather than the base primitive. Reply with "@CharlieHelps yes please" if you'd like me to add a commit with this suggestion.
| <ThreadHistoryBase.Root | ||
| ref={ref} | ||
| onThreadChange={onThreadChange} | ||
| defaultCollapsed={defaultCollapsed} | ||
| position={position} | ||
| className={cn( | ||
| "border-flat bg-container h-full transition-all duration-300 flex-none", | ||
| position === "left" ? "border-r" : "border-l", | ||
| className, | ||
| )} | ||
| {...props} | ||
| > | ||
| <div | ||
| ref={ref} | ||
| className={cn( | ||
| "border-flat bg-container h-full transition-all duration-300 flex-none", | ||
| position === "left" ? "border-r" : "border-l", | ||
| isCollapsed ? "w-12" : "w-64", | ||
| className, | ||
| )} | ||
| {...props} | ||
| > | ||
| {({ isCollapsed }) => ( | ||
| <div | ||
| className={cn( | ||
| "flex flex-col h-full", | ||
| isCollapsed ? "py-4 px-2" : "p-4", | ||
| )} // py-4 px-2 is for better alignment when isCollapsed | ||
| )} | ||
| > | ||
| {children} | ||
| </div> | ||
| </div> | ||
| </ThreadHistoryContext.Provider> | ||
| )} | ||
| </ThreadHistoryBase.Root> |
There was a problem hiding this comment.
Styled wrapper dropped explicit width handling
Previously the styled wrapper applied isCollapsed ? "w-12" : "w-64". After refactor, the width class is no longer applied in the wrapper; instead, the base root sets a global --sidebar-width variable. Unless the layout CSS elsewhere actually uses var(--sidebar-width), this change can break the visual collapse/expand behavior.
Even if it works today, it makes the wrapper’s sizing implicit and harder to reason about.
Suggestion
Keep width behavior local to the styled wrapper so it’s explicit and doesn’t rely on global CSS variables:
<ThreadHistoryBase.Root ...>
{({ isCollapsed }) => (
<div
className={cn(
"border-flat bg-container h-full transition-all duration-300 flex-none",
position === "left" ? "border-r" : "border-l",
isCollapsed ? "w-12" : "w-64",
className,
)}
>
...
</div>
)}
</ThreadHistoryBase.Root>(or ensure the component actually consumes --sidebar-width in its styles).
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.
ThreadHistorySearchInput uses a fixed setTimeout(300) to focus, which is brittle
The focus logic assumes an animation duration and schedules focus 300ms later. That can easily desync if styles change, and can lead to focusing an element that has been unmounted.
Also, there’s no cancellation/cleanup of the timeout on unmount or on rapid toggles.
Suggestion
Prefer focusing on the next frame(s) and clean up pending timers. If you want to wait for expansion, use requestAnimationFrame twice or a timeout you cancel.
const focusTimerRef = React.useRef<number | null>(null);
React.useEffect(() => {
return () => {
if (focusTimerRef.current != null) {
window.clearTimeout(focusTimerRef.current);
}
};
}, []);
const expandOnSearch = React.useCallback(() => {
if (!isCollapsed) {
searchInputRef.current?.focus();
return;
}
setIsCollapsed(false);
if (focusTimerRef.current != null) window.clearTimeout(focusTimerRef.current);
focusTimerRef.current = window.setTimeout(() => {
searchInputRef.current?.focus();
}, 0);
}, [isCollapsed, setIsCollapsed]);Reply with "@CharlieHelps yes please" if you'd like me to add a commit with this suggestion.
3d02498 to
b753c1c
Compare
7467ef3 to
5a90cd3
Compare
b753c1c to
c07cab3
Compare
Fixes TAM-1064 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
… render prop in thread-history
5a90cd3 to
8fac220
Compare
Summary
Adds
thread-historycompound component base primitives and styled wrapper.Base: Root, Header, List, Item, CollapseToggle, NewThreadButton, SearchInput
Styled: Composes base components with Tailwind styling
Fixes TAM-1064
Test Plan
npm run lint && npm run check-types && npm test🤖 Generated with Claude Code