feat(react-ui-base): add thread-dropdown compound component#2278
feat(react-ui-base): add thread-dropdown compound component#2278lachieh 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 ThreadDropdown primitives are generally well-factored, but ThreadDropdownRoot currently installs a global keyboard shortcut without scoping/guards or an opt-out, which is risky in a base library. The Trigger also unconditionally passes type="button" even when asChild is used, producing invalid attributes on non-button elements. There are also some ARIA/semantics concerns (role="menu") and a mismatch between the base API and the styled wrapper (base ThreadItem isn’t used).
Additional notes (2)
-
Maintainability |
...ges/react-ui-base/src/thread-dropdown/root/thread-dropdown-root.tsx:10-58
ThreadDropdownRootPropsdefinesonThreadChange?: () => void, but the callback is invoked in two different situations: -
after
startNewThread()completes -
immediately after calling
switchCurrentThread(threadId)
For consumers, it’s hard to know what changed to what. This often leads to refetching everything or relying on other global state.
Also, switchCurrentThread is called inside a try/catch, but if it’s async (or becomes async later), the try/catch won’t catch promise rejections and onThreadChange could fire before the switch actually completes.
- Maintainability |
...ase/src/thread-dropdown/thread-item/thread-dropdown-thread-item.tsx:31-66
ThreadDropdownThreadItemcomputes a defaultthreadLabelviathread.id.substring(0, 8). If IDs can be shorter than 8 chars this is fine, but if IDs can be non-stringy or formatted differently, you may want the base primitive to be less opinionated.
More importantly: the styled wrapper does not use ThreadDropdownBase.ThreadItem at all; it maps threads and renders Radix items directly. That makes the ThreadItem primitive effectively dead weight for the primary consumer shown here, and makes the base API inconsistent (sometimes render-props, sometimes direct mapping).
Summary of changes
✅ Added ThreadDropdown compound component to react-ui-base
- Added a new package export entry for
./thread-dropdowninpackages/react-ui-base/package.json(CJS/ESM + types). - Exported the new namespace component and related types from
packages/react-ui-base/src/index.ts. - Introduced new headless primitives under
packages/react-ui-base/src/thread-dropdown/*:Root(context + data fetching + keyboard shortcut)TriggerMenuThreadItemNewThreadItem
- Implemented shared context (
ThreadDropdownContext) and hook (useThreadDropdownContext).
✅ Updated ui-registry styled wrapper to use base primitives
- Reworked
packages/ui-registry/src/components/thread-dropdown/thread-dropdown.tsxto compose@tambo-ai/react-ui-base/thread-dropdownprimitives with RadixDropdownMenuand Tailwind styling. - Updated
config.jsondependencies to include@tambo-ai/react-ui-base@nextand removedclass-variance-authority.
| 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 installs a global document.addEventListener("keydown", ...) regardless of whether the dropdown is open, and regardless of where the component is rendered. This creates a few concrete problems:
- Behavioral conflicts:
Alt+Shift+Nwill fire even while the user is typing in an<input>/<textarea>or using the shortcut elsewhere. - Multi-mount duplication: Rendering more than one
ThreadDropdown.Root(e.g., multiple panels) will register multiple listeners and can trigger multiple thread creations. - Accessibility/UX: Global shortcuts should generally be scoped (only when menu is open/focused) or at least ignore editable targets.
Given this is a base primitive in react-ui-base, it’s especially important not to impose a global shortcut with no opt-out.
Suggestion
Consider scoping or hardening the shortcut handling:
- Ignore events originating from editable elements.
- Allow opting out via a prop like
enableKeyboardShortcut?: boolean(defaulttrue) orshortcut?: { enabled: boolean; combo: ... }. - Optionally only bind while the dropdown/menu is open (if you can accept an
openstate prop or integrate with Radix in the styled layer instead of base).
Example (minimal hardening):
React.useEffect(() => {
const handleKeyDown = (event: KeyboardEvent) => {
const target = event.target as HTMLElement | null;
const isEditable =
target?.tagName === "INPUT" ||
target?.tagName === "TEXTAREA" ||
(target as HTMLElement | null)?.isContentEditable;
if (isEditable) 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 implementing an opt-out prop + editable-target guard.
| export const ThreadDropdownTrigger = React.forwardRef< | ||
| HTMLButtonElement, | ||
| ThreadDropdownTriggerProps | ||
| >(({ asChild, children, ...props }, ref) => { | ||
| const Comp = asChild ? Slot : "button"; | ||
|
|
||
| return ( | ||
| <Comp | ||
| ref={ref} | ||
| type="button" | ||
| data-slot="thread-dropdown-trigger" | ||
| {...props} | ||
| > | ||
| {children} | ||
| </Comp> | ||
| ); |
There was a problem hiding this comment.
ThreadDropdownTrigger unconditionally sets type="button". When asChild is used (as it is in ui-registry via Radix DropdownMenu.Trigger asChild), the child is a Slot and may ultimately render a non-<button> element. Passing a type attribute to a non-button element is invalid HTML and can create confusing DOM output.
This is a common gotcha with asChild patterns: element-specific attributes should only be applied when you control the underlying tag.
Suggestion
Only apply type="button" when rendering an actual button, or gate it behind !asChild.
export const ThreadDropdownTrigger = React.forwardRef<
HTMLButtonElement,
ThreadDropdownTriggerProps
>(({ asChild, children, ...props }, ref) => {
const Comp = asChild ? Slot : "button";
return (
<Comp
ref={ref}
{...(!asChild ? { type: "button" } : {})}
data-slot="thread-dropdown-trigger"
{...props}
>
{children}
</Comp>
);
});Reply with "@CharlieHelps yes please" if you'd like me to add a commit with this fix.
| return ( | ||
| <Comp | ||
| ref={ref} | ||
| data-slot="thread-dropdown-menu" | ||
| role="menu" | ||
| {...componentProps} | ||
| > | ||
| {content} | ||
| </Comp> | ||
| ); |
There was a problem hiding this comment.
The base ThreadDropdownMenu sets role="menu", while in the styled wrapper the actual interactive menu is Radix DropdownMenu.Content / DropdownMenu.Item, which already applies appropriate roles/ARIA.
This can lead to nested menu roles or conflicting semantics depending on how consumers compose ThreadDropdown.Menu (right now it’s used purely as a render-props provider, not as a DOM wrapper with its own menu semantics).
Suggestion
If ThreadDropdownMenu is primarily a data/render-props provider, consider not forcing role="menu" at the base level. Alternatively, only set role when not asChild, or document that it should wrap actual menu content.
Minimal change:
<Comp
ref={ref}
data-slot="thread-dropdown-menu"
{...(!asChild ? { role: "menu" } : {})}
{...componentProps}
>Reply with "@CharlieHelps yes please" if you'd like me to add a commit implementing this guard.
3d02498 to
b753c1c
Compare
1442fa1 to
b48f9d2
Compare
b753c1c to
c07cab3
Compare
Fixes TAM-1063 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
… render prop in thread-dropdown
b48f9d2 to
a82e5bd
Compare
Summary
Adds
thread-dropdowncompound component base primitives and styled wrapper.Base: Root, Trigger, Menu, ThreadItem, NewThreadItem
Styled: Composes base components with Tailwind styling
Fixes TAM-1063
Test Plan
npm run lint && npm run check-types && npm test🤖 Generated with Claude Code