feat(react-ui-base): add message-thread-collapsible base primitive#2284
feat(react-ui-base): add message-thread-collapsible base primitive#2284lachieh 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.
MessageThreadCollapsibleTriggercurrently allows consumeronClickto override the internal toggle due to prop spread order, which can break the core behavior.- Hotkey parsing/keyboard matching in
MessageThreadCollapsibleRootis brittle (case/format) and can lead to unreliable shortcuts. useCanvasDetectioncan become stale when the canvas element is mounted/removed or repositioned without a window resize; consider observing DOM/layout changes more directly.
Summary of changes
Summary
This PR introduces a new unstyled base primitive for a collapsible message thread, plus shared layout utilities.
New exports / entrypoints
- Added a new package export path:
./message-thread-collapsibleinpackages/react-ui-base/package.json. - Re-exported the namespace component and types from
packages/react-ui-base/src/index.ts.
New MessageThreadCollapsible primitive
- Added a compound component namespace
MessageThreadCollapsiblewith:Root(state + hotkey registration)Trigger(toggle button)Header(header area + close callback)Content(conditionally rendered when open)
- Added an internal context (
MessageThreadCollapsibleContext) to share state and helpers.
New layout utility hooks
useCanvasDetection(elementRef)to detect a[data-canvas-space="true"]element and whether it is left of a referenced element.usePositioning(className, canvasIsOnLeft, hasCanvasSpace)and helperhasRightClass()to computehistoryPositionand whether a panel is left/right.
| return ( | ||
| <Comp | ||
| ref={ref} | ||
| type="button" | ||
| data-slot="message-thread-collapsible-trigger" | ||
| data-state={isOpen ? "open" : "closed"} | ||
| onClick={() => setIsOpen(!isOpen)} | ||
| {...componentProps} | ||
| > | ||
| {content} |
There was a problem hiding this comment.
Trigger is overriding/ignoring consumer-provided onClick coming from componentProps. Because {...componentProps} is spread after onClick, any onClick passed by the consumer will replace the internal toggle logic, making the trigger stop toggling.
This is a correctness bug for a base primitive: it should always toggle, while still allowing user handlers to run.
Suggestion
Update the click handling to compose handlers rather than allowing componentProps.onClick to override the toggle. Also use a functional state update to avoid stale closures.
return (
<Comp
ref={ref}
type="button"
data-slot="message-thread-collapsible-trigger"
data-state={isOpen ? "open" : "closed"}
{...componentProps}
onClick={(e: React.MouseEvent<HTMLButtonElement>) => {
componentProps.onClick?.(e);
if (e.defaultPrevented) return;
setIsOpen((prev) => !prev);
}}
>
{content}
</Comp>
);Reply with "@CharlieHelps yes please" if you'd like me to add a commit with this suggestion.
| const shortcutText = React.useMemo(() => { | ||
| const [modifier, key] = hotkey.split("+"); | ||
| let modDisplay = ""; | ||
| if (modifier === "mod") { | ||
| modDisplay = isMac ? "⌘" : "Ctrl+"; | ||
| } | ||
| return `${modDisplay}${(key ?? "").toUpperCase()}`; | ||
| }, [hotkey, isMac]); | ||
|
|
||
| React.useEffect(() => { | ||
| const down = (e: KeyboardEvent) => { | ||
| const [modifier, key] = hotkey.split("+"); | ||
| const isModifierPressed = | ||
| modifier === "mod" ? e.metaKey || e.ctrlKey : false; | ||
| if (e.key === key && isModifierPressed) { | ||
| e.preventDefault(); | ||
| setIsOpen((prev) => !prev); | ||
| } | ||
| }; | ||
| document.addEventListener("keydown", down); | ||
| return () => document.removeEventListener("keydown", down); | ||
| }, [hotkey]); |
There was a problem hiding this comment.
The hotkey handler compares e.key === key without normalizing case and without guarding against empty/invalid hotkey formats. In practice, KeyboardEvent.key is typically lowercase for letter keys (e.g. 'k'), while a configured hotkey could be 'mod+K' or include extra whitespace. That makes the shortcut flaky across configurations.
Also, splitting on + and only supporting mod is fine, but it should fail safely (no handler firing) and be robust to minor formatting differences.
Suggestion
Normalize and validate the parsed hotkey once (e.g., in a useMemo) and compare using a consistent case.
const parsedHotkey = React.useMemo(() => {
const [rawMod, rawKey] = hotkey.split("+").map((s) => s.trim().toLowerCase());
return { mod: rawMod, key: rawKey };
}, [hotkey]);
React.useEffect(() => {
const down = (e: KeyboardEvent) => {
if (!parsedHotkey.key) return;
const isModifierPressed =
parsedHotkey.mod === "mod" ? e.metaKey || e.ctrlKey : false;
if (e.key.toLowerCase() === parsedHotkey.key && isModifierPressed) {
e.preventDefault();
setIsOpen((prev) => !prev);
}
};
document.addEventListener("keydown", down);
return () => document.removeEventListener("keydown", down);
}, [parsedHotkey]);Reply with "@CharlieHelps yes please" if you'd like me to add a commit with this suggestion.
| const shortcutText = React.useMemo(() => { | ||
| const [modifier, key] = hotkey.split("+"); | ||
| let modDisplay = ""; | ||
| if (modifier === "mod") { | ||
| modDisplay = isMac ? "⌘" : "Ctrl+"; | ||
| } | ||
| return `${modDisplay}${(key ?? "").toUpperCase()}`; | ||
| }, [hotkey, isMac]); | ||
|
|
||
| React.useEffect(() => { | ||
| const down = (e: KeyboardEvent) => { | ||
| const [modifier, key] = hotkey.split("+"); | ||
| const isModifierPressed = | ||
| modifier === "mod" ? e.metaKey || e.ctrlKey : false; | ||
| if (e.key === key && isModifierPressed) { | ||
| e.preventDefault(); | ||
| setIsOpen((prev) => !prev); | ||
| } | ||
| }; | ||
| document.addEventListener("keydown", down); | ||
| return () => document.removeEventListener("keydown", down); | ||
| }, [hotkey]); |
There was a problem hiding this comment.
The hotkey parsing/handling has a few correctness and UX issues:
hotkey.split("+")assumes exactlymodifier+key. For values like"mod+shift+k"or"ctrl+alt+k", it will misparse.e.key === keyis case/layout sensitive and doesn’t normalize. For exampleKvskor non-letter keys.- The handler doesn’t ignore events from editable targets (
input,textarea,[contenteditable]). This can unexpectedly toggle while typing.
Given this is a base primitive, these edge cases will show up quickly in apps.
Suggestion
Either (A) switch to a dedicated hotkey library used elsewhere in the repo (if available), or (B) harden parsing + event guards.
Minimal hardening approach:
- Parse
hotkeyinto tokens and supportmod,shift,alt,ctrlplus a final key. - Normalize via
e.key.toLowerCase(). - Ignore keydowns originating from editable elements.
Example:
function isEditableTarget(t: EventTarget | null) {
if (!(t instanceof Element)) return false;
const tag = t.tagName;
return (
tag === "INPUT" ||
tag === "TEXTAREA" ||
tag === "SELECT" ||
t.getAttribute("contenteditable") === "true" ||
t.closest("[contenteditable='true']") != null
);
}
function parseHotkey(hotkey: string) {
const parts = hotkey.toLowerCase().split("+").filter(Boolean);
const key = parts.pop() ?? "";
const mods = new Set(parts);
return { key, mods };
}
React.useEffect(() => {
const { key, mods } = parseHotkey(hotkey);
const down = (e: KeyboardEvent) => {
if (isEditableTarget(e.target)) return;
const wantsMod = mods.has("mod");
const modOk = !wantsMod || e.metaKey || e.ctrlKey;
const shiftOk = !mods.has("shift") || e.shiftKey;
const altOk = !mods.has("alt") || e.altKey;
const ctrlOk = !mods.has("ctrl") || e.ctrlKey;
if (modOk && shiftOk && altOk && ctrlOk && e.key.toLowerCase() === key) {
e.preventDefault();
setIsOpen((prev) => !prev);
}
};
document.addEventListener("keydown", down);
return () => document.removeEventListener("keydown", down);
}, [hotkey]);Reply with "@CharlieHelps yes please" if you'd like me to add a commit with this suggestion.
| const isMac = | ||
| typeof navigator !== "undefined" && navigator.platform.startsWith("Mac"); | ||
|
|
||
| const shortcutText = React.useMemo(() => { | ||
| const [modifier, key] = hotkey.split("+"); | ||
| let modDisplay = ""; | ||
| if (modifier === "mod") { | ||
| modDisplay = isMac ? "⌘" : "Ctrl+"; | ||
| } | ||
| return `${modDisplay}${(key ?? "").toUpperCase()}`; | ||
| }, [hotkey, isMac]); | ||
|
|
There was a problem hiding this comment.
navigator.platform is deprecated in some environments and can produce surprising results; startsWith("Mac") is also a brittle heuristic. If isMac is wrong, shortcutText becomes misleading.
Since this is purely for display, consider using navigator.userAgentData?.platform when available, with a fallback to a UA/platform heuristic, or accept an optional prop override for shortcutText/isMac.
Suggestion
Prefer navigator.userAgentData?.platform when present, and fall back to navigator.platform.
const isMac = React.useMemo(() => {
if (typeof navigator === "undefined") return false;
const platform =
(navigator as any).userAgentData?.platform ?? navigator.platform;
return typeof platform === "string" && /mac/i.test(platform);
}, []);Optionally add shortcutText?: string prop to bypass platform detection.
Reply with "@CharlieHelps yes please" if you'd like me to add a commit with this suggestion.
| React.useEffect(() => { | ||
| const checkCanvas = () => { | ||
| const canvas = document.querySelector('[data-canvas-space="true"]'); | ||
| setHasCanvasSpace(!!canvas); | ||
|
|
||
| if (canvas && elementRef.current) { | ||
| const canvasRect = canvas.getBoundingClientRect(); | ||
| const elemRect = elementRef.current.getBoundingClientRect(); | ||
| setCanvasIsOnLeft(canvasRect.left < elemRect.left); | ||
| } | ||
| }; | ||
|
|
||
| checkCanvas(); | ||
| const timeoutId = setTimeout(checkCanvas, 100); | ||
|
|
||
| window.addEventListener("resize", checkCanvas); | ||
|
|
||
| return () => { | ||
| clearTimeout(timeoutId); | ||
| window.removeEventListener("resize", checkCanvas); | ||
| }; | ||
| }, [elementRef]); |
There was a problem hiding this comment.
useCanvasDetection attaches a resize listener and also schedules a setTimeout check, but it does not respond to changes in the canvas element being added/removed or repositioned due to DOM/layout changes unrelated to window resize.
Given this is a shared layout utility used across variants, the current behavior may become stale in common UI transitions (e.g. canvas mounting after data fetch, panel toggles, CSS transitions).
Suggestion
Consider adding a MutationObserver (or at least re-checking on requestAnimationFrame when elementRef.current becomes available) to keep detection accurate when [data-canvas-space="true"] is mounted/removed without a resize.
Example using MutationObserver:
React.useEffect(() => {
const checkCanvas = () => {
const canvas = document.querySelector('[data-canvas-space="true"]');
setHasCanvasSpace(!!canvas);
if (canvas && elementRef.current) {
const canvasRect = canvas.getBoundingClientRect();
const elemRect = elementRef.current.getBoundingClientRect();
setCanvasIsOnLeft(canvasRect.left < elemRect.left);
}
};
checkCanvas();
const observer = new MutationObserver(checkCanvas);
observer.observe(document.body, { childList: true, subtree: true, attributes: true });
window.addEventListener("resize", checkCanvas);
return () => {
observer.disconnect();
window.removeEventListener("resize", checkCanvas);
};
}, [elementRef]);Reply with "@CharlieHelps yes please" if you'd like me to add a commit with this suggestion.
| React.useEffect(() => { | ||
| const checkCanvas = () => { | ||
| const canvas = document.querySelector('[data-canvas-space="true"]'); | ||
| setHasCanvasSpace(!!canvas); | ||
|
|
||
| if (canvas && elementRef.current) { | ||
| const canvasRect = canvas.getBoundingClientRect(); | ||
| const elemRect = elementRef.current.getBoundingClientRect(); | ||
| setCanvasIsOnLeft(canvasRect.left < elemRect.left); | ||
| } | ||
| }; | ||
|
|
||
| checkCanvas(); | ||
| const timeoutId = setTimeout(checkCanvas, 100); | ||
|
|
||
| window.addEventListener("resize", checkCanvas); | ||
|
|
||
| return () => { | ||
| clearTimeout(timeoutId); | ||
| window.removeEventListener("resize", checkCanvas); | ||
| }; | ||
| }, [elementRef]); |
There was a problem hiding this comment.
useCanvasDetection uses document.querySelector + getBoundingClientRect and calls setState on mount, after a timeout, and on every resize. This can cause layout thrash and a lot of re-rendering during window resize (potential performance issue), especially if multiple components use the hook.
Also, the effect depends on [elementRef], but RefObjects are stable; what you actually care about is changes to elementRef.current (which won’t retrigger the effect).
Suggestion
Reduce work and improve correctness:
- Use
requestAnimationFramethrottling for resize. - Consider a
ResizeObserverforelementRef.currentand aMutationObserverfor the canvas element if it can appear/disappear dynamically. - Make the effect run once (
[]) and readelementRef.currentinside.
Minimal improvement with rAF throttle + stable deps:
React.useEffect(() => {
let raf = 0;
const checkCanvas = () => {
const canvas = document.querySelector('[data-canvas-space="true"]');
setHasCanvasSpace(!!canvas);
const el = elementRef.current;
if (canvas && el) {
const canvasRect = canvas.getBoundingClientRect();
const elemRect = el.getBoundingClientRect();
setCanvasIsOnLeft(canvasRect.left < elemRect.left);
}
};
const onResize = () => {
cancelAnimationFrame(raf);
raf = requestAnimationFrame(checkCanvas);
};
checkCanvas();
const timeoutId = window.setTimeout(checkCanvas, 100);
window.addEventListener("resize", onResize);
return () => {
window.clearTimeout(timeoutId);
cancelAnimationFrame(raf);
window.removeEventListener("resize", onResize);
};
}, []);Reply with "@CharlieHelps yes please" if you'd like me to add a commit with this suggestion.
| if (!isOpen) { | ||
| return null; | ||
| } | ||
|
|
There was a problem hiding this comment.
MessageThreadCollapsibleContent unmounts children when closed (return null). If consumers put stateful components inside Content (inputs, editors, virtualization, etc.), closing will reset state and may be undesirable. For a base primitive, it’s usually better to support both behaviors.
At minimum, this behavior should be explicitly controllable (e.g. forceMount like Radix) or exposed via a prop.
Suggestion
Add an optional forceMount?: boolean prop (default false) and render with hidden/display:none semantics when closed if forceMount is true.
export interface MessageThreadCollapsibleContentOwnProps {
forceMount?: boolean;
}
export type MessageThreadCollapsibleContentProps = BasePropsWithChildrenOrRenderFunction<
React.HTMLAttributes<HTMLDivElement> & MessageThreadCollapsibleContentOwnProps,
MessageThreadCollapsibleContentRenderProps
>;
// ...
if (!isOpen && !componentProps.forceMount) return null;
return (
<Comp
ref={ref}
data-slot="message-thread-collapsible-content"
hidden={!isOpen}
{...componentProps}
>
{content}
</Comp>
);Reply with "@CharlieHelps yes please" if you'd like me to add a commit with this suggestion.
3d02498 to
b753c1c
Compare
5de782c to
57b7ba6
Compare
b753c1c to
c07cab3
Compare
Extract collapsible thread layout logic into unstyled base components: - Root: manages open/closed state and keyboard shortcut registration - Trigger: toggle button with open/closed data attributes - Header: header area visible when open, with close callback - Content: conditionally rendered content area Also adds shared layout utility hooks (useCanvasDetection, usePositioning) used across all message-thread-* variants. Fixes TAM-1058 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…sible - Add aria-expanded/aria-controls on Trigger, id/role on Content - Memoize context value to prevent unnecessary re-renders - Add hotkey format validation (fail-fast) - Fix deprecated navigator.platform with userAgent fallback - Derive RootRenderProps from ContextValue (eliminate duplication) - Memoize onClose callback in Header - Add displayName on Trigger, Header, Content - Eliminate let usage via resolveContent helper - Mark render prop as @deprecated
57b7ba6 to
12a7e16
Compare
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
Extract collapsible thread layout logic into unstyled base components:
Also adds shared layout utility hooks (useCanvasDetection, usePositioning) used across all message-thread-* variants.
Fixes TAM-1058