[WEB-4682 | WEB-4685] feat: propel comobobox and command component #7615
[WEB-4682 | WEB-4685] feat: propel comobobox and command component #7615sriramveeraghanta merged 7 commits intopreviewfrom
Conversation
WalkthroughAdds two public exports ("./command" and "./combobox") to packages/propel and introduces new Command and Combobox React modules with barrel index files. Combobox is a compound component supporting controlled/uncontrolled usage and multi-select; no dependency changes. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Btn as Combobox.Button
participant Pop as Popover
participant Cmd as Command (Input/List)
participant Ctx as ComboboxContext
participant App as onValueChange
User->>Btn: click (toggle)
Btn->>Pop: toggle open
Pop-->>User: show options
User->>Cmd: type / navigate
Cmd->>Cmd: filter options
User->>Cmd: select item
Cmd->>Ctx: handleValueChange(item)
alt multiSelect
Ctx->>Ctx: toggle selection (respect maxSelections)
Ctx-->>App: onValueChange(newValues)
else singleSelect
Ctx->>Pop: close
Ctx-->>App: onValueChange(newValue)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 📜 Recent review detailsConfiguration used: CodeRabbit UI 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (7)
packages/propel/src/command/command.tsx (3)
6-8: Nit: remove no-op cn("") usage.
cn("", className)is equivalent tocn(className).- return <CommandPrimitive data-slot="command" className={cn("", className)} {...props} />; + return <CommandPrimitive data-slot="command" className={cn(className)} {...props} />;
4-4: Consistency: align cn import with other modules.Combobox imports
cnfrom@plane/utils. Consider standardizing on a single source to avoid divergence.-import { cn } from "@plane/ui"; +import { cn } from "@plane/utils";
10-20: Consider forwarding refs for better ergonomics.Exposing refs on
Command,Command.Input, andCommand.Itemimproves focus management and integrability with forms.Example for
Command.Input:const CommandInput = React.forwardRef< React.ElementRef<typeof CommandPrimitive.Input>, React.ComponentPropsWithoutRef<typeof CommandPrimitive.Input> >(({ className, ...props }, ref) => ( <div data-slot="command-input-wrapper" className="flex items-center gap-1.5 rounded border border-custom-border-100 bg-custom-background-90 px-2"> <SearchIcon className="size-3.5 flex-shrink-0 text-custom-text-400" strokeWidth={1.5} /> <CommandPrimitive.Input ref={ref} data-slot="command-input" className={cn(className)} {...props} /> </div> ));packages/propel/src/combobox/combobox.tsx (4)
284-287: Minor: keep explicit type parity with cmdk.No behavior change; ensures
onSelectreceives astringand keeps props tidy after the type change.- return ( - <Command.Item value={stringValue} disabled={isDisabled} onSelect={handleValueChange} className={className}> - {children} - </Command.Item> - ); + return ( + <Command.Item + value={stringValue} + disabled={isDisabled} + onSelect={handleValueChange} + className={className} + > + {children} + </Command.Item> + );
6-12: Dead API surface: exported ComboboxOption interface is unused.
ComboboxOption(withquery,content,tooltip) isn’t referenced by the components. Either wire it into the API (e.g., accept anoptions: ComboboxOption[]) or drop the export to avoid confusion.
32-42: Unused prop: showCheckIcon is defined but never used.If keeping it, render a checkmark for selected items in
ComboboxOptionwhenmultiSelectis true.I can draft a minimal implementation that conditionally shows a check icon without imposing styling, if you want to include it in this PR.
2-4: Create a barrel forCommandto enable consistent importsThe
packages/propel/src/commanddirectory currently lacks anindex.tsbarrel, so importing viaimport { Command } from "../command";would fail. To align with your existing Popover barrel and keep imports resilient to future file renames, consider:
- Adding a barrel file at
packages/propel/src/command/index.tswith:export * from "./command";- Updating your imports in
combobox.tsxaccordingly:- import { Command } from "../command/command"; + import { Command } from "../command"; - import { Popover } from "../popover/root"; + import { Popover } from "../popover";The Popover barrel already exists (
packages/propel/src/popover/index.tsexports from./root), so no extra setup is needed there.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
packages/propel/package.json(1 hunks)packages/propel/src/combobox/combobox.tsx(1 hunks)packages/propel/src/combobox/index.ts(1 hunks)packages/propel/src/command/command.tsx(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build and lint web apps
- GitHub Check: Build and lint web apps
- GitHub Check: Analyze (javascript)
🔇 Additional comments (3)
packages/propel/src/combobox/index.ts (1)
1-1: Barrel export looks good.Keeps the public surface simple and tree-shakeable.
packages/propel/src/combobox/combobox.tsx (2)
39-40: Naming: confirm maxHeight token "rg"."rg" is unconventional next to "lg | md | sm". If intentional ("regular"), OK; otherwise consider aligning with a standard scale (e.g., "xl | lg | md | sm").
231-263: A11y: ensure Popover.Panel semantics match listbox expectations.You’ve set
role="listbox"andaria-multiselectableonCommand.List, which is good. Double-check thatPopover.Paneldoesn’t add conflicting roles and that focus is correctly moved to the list or first item when opened (Radix-style APIs sometimes requirearia-controls/focus management).
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/propel/src/combobox/combobox.tsx (1)
270-275: Remove the redundant alias and clean up memo deps.
const stringValue = value;adds noise and leaks into dependency arrays. Usevaluedirectly in comparisons and props.Apply:
- const stringValue = value; const isSelected = React.useMemo(() => { if (!multiSelect) return false; - return Array.isArray(selectedValue) ? (selectedValue as string[]).includes(stringValue) : false; - }, [multiSelect, selectedValue, stringValue]); + return Array.isArray(selectedValue) ? (selectedValue as string[]).includes(value) : false; + }, [multiSelect, selectedValue, value]); - return ( - <Command.Item value={stringValue} disabled={isDisabled} onSelect={handleValueChange} className={className}> + return ( + <Command.Item value={value} disabled={isDisabled} onSelect={handleValueChange} className={className}> {children} </Command.Item> );
🧹 Nitpick comments (5)
packages/propel/src/combobox/combobox.tsx (5)
6-12: Unify option value typing with the string-based API.
ComboboxOption.valueisunknown, while the rest of the component surface is string-only (ComboboxProps.value: string | string[],ComboboxOptionProps.value: string). Keeping this asunknowninvites lossy coercion and identity collisions downstream. Align this tostring(or remove the unused interface if it isn’t part of the public API yet).Apply:
export interface ComboboxOption { - value: unknown; + value: string; query: string; content: React.ReactNode; disabled?: boolean; tooltip?: string | React.ReactNode; }
32-42: Implement showCheckIcon (currently unused) via a lightweight Options context.
ComboboxOptionsProps.showCheckIconis declared but never read; consumers can’t actually toggle selection indicators. Recommend threading this value via a local context provided byComboboxOptionsand consumed byComboboxOption, rendering a checkmark for the selected item(s).Diffs within the shown ranges:
function ComboboxOptions({ children, showSearch = false, searchPlaceholder, maxHeight, className, inputClassName, optionsContainerClassName, - emptyMessage, + emptyMessage, + showCheckIcon, }: ComboboxOptionsProps) { const { multiSelect } = useComboboxContext(); return ( - <Popover.Panel sideOffset={8} className={cn(className)}> + <OptionsSettingsContext.Provider value={{ showCheckIcon }}> + <Popover.Panel sideOffset={8} className={cn(className)}> <Command> {showSearch && <Command.Input placeholder={searchPlaceholder} className={cn(inputClassName)} />} <Command.List className={cn( { "max-h-60": maxHeight === "lg", "max-h-48": maxHeight === "md", "max-h-36": maxHeight === "rg", "max-h-28": maxHeight === "sm", }, optionsContainerClassName )} role="listbox" aria-multiselectable={multiSelect || undefined} > {children} </Command.List> <Command.Empty>{emptyMessage ?? "No options found."}</Command.Empty> </Command> - </Popover.Panel> + </Popover.Panel> + </OptionsSettingsContext.Provider> ); }And within
ComboboxOption(context consumption + indicator, see separate diff in the comment below for the surrounding lines), add:+ const { showCheckIcon } = React.useContext(OptionsSettingsContext); + const singleSelected = + !multiSelect && !Array.isArray(selectedValue) && selectedValue === value; + const showIndicator = !!showCheckIcon && (multiSelect ? isSelected : singleSelected);Then render an indicator:
return ( - <Command.Item value={value} disabled={isDisabled} onSelect={handleValueChange} className={className}> - {children} - </Command.Item> + <Command.Item value={value} disabled={isDisabled} onSelect={handleValueChange} className={className}> + {showIndicator && <span aria-hidden="true" className="mr-2">✓</span>} + {children} + </Command.Item> );Outside of the shown ranges (add once near other context declarations):
// Add near ComboboxContext const OptionsSettingsContext = React.createContext<{ showCheckIcon?: boolean }>({});Also applies to: 231-265
215-229: A11y: Consider linking button and listbox via aria-controls/id.Optional but recommended for the button+listbox pattern: give the listbox an
idand reference it from the trigger witharia-controls. Since Button and Options are siblings, the simplest approach is to generate an id in the root and share via context.Sketch:
interface ComboboxContextType { value: string | string[]; onValueChange?: (value: string | string[]) => void; multiSelect: boolean; maxSelections?: number; disabled: boolean; open: boolean; setOpen: (open: boolean) => void; handleValueChange: (newValue: string) => void; handleRemoveSelection: (valueToRemove: string) => void; + listboxId: string; }function ComboboxComponent({...}: ComboboxProps) { + const listboxId = React.useId(); ... const contextValue = React.useMemo<ComboboxContextType>( () => ({ value: internalValue, onValueChange, multiSelect, maxSelections, disabled, open, setOpen, handleValueChange, handleRemoveSelection, + listboxId, }), [/* existing deps */, /* + */ listboxId] );function ComboboxButton({...}) { - const { disabled: ctxDisabled, open } = useComboboxContext(); + const { disabled: ctxDisabled, open, listboxId } = useComboboxContext(); ... <Popover.Button ... - aria-expanded={open} + aria-expanded={open} + aria-controls={listboxId} ><Command.List ... + id={listboxId} role="listbox" aria-multiselectable={multiSelect || undefined} >Also applies to: 246-257
39-41: Nit: size token “rg”.The size alias
rgis uncommon alongsidesm | md | lg. If this is meant to be “regular,” consider usingbase/mdor addingxl | lg | md | smfor consistency with existing components. Otherwise, documentrgin the props JSDoc.Would you like me to align this with an existing design token set in the codebase if there is one?
Also applies to: 249-253
85-90: Clarify multiSelect contract or normalize state shapeA search across all
.tsxfiles found no instances where<Combobox multiSelect={…}>is toggled dynamically at runtime. While this makes breakage unlikely in the current codebase, it’s still best to explicitly document the expected stability of themultiSelectprop and optionally safeguard uncontrolled usage against late flips:• Document that the
multiSelectprop is static for the component’s lifetime.
• (optional) Add an effect to normalize the internal state shape whenmultiSelectchanges in uncontrolled mode:// Update internal value when prop changes React.useEffect(() => { if (isControlledValue) { setInternalValue(value as string | string[]); } }, [isControlledValue, value]); + // Normalize value shape if multiSelect flips during lifetime (uncontrolled only) + React.useEffect(() => { + if (isControlledValue) return; + setInternalValue((prev) => { + // single -> multi + if (multiSelect && !Array.isArray(prev)) { + const next = prev ? [prev as string] : []; + onValueChange?.(next); + return next; + } + // multi -> single + if (!multiSelect && Array.isArray(prev)) { + const [first = ""] = prev as string[]; + onValueChange?.(first); + return first; + } + return prev; + }); + }, [multiSelect, isControlledValue, onValueChange]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
packages/propel/src/combobox/combobox.tsx(1 hunks)
🔇 Additional comments (1)
packages/propel/src/combobox/combobox.tsx (1)
239-262: LGTM: emptyMessage is now respected.
<Command.Empty>now rendersemptyMessage ?? "No options found.", enabling customization without breaking defaults.
…om:makeplane/plane into feat-propel-command-and-combobox-component
sriramveeraghanta
deleted the
feat-propel-command-and-combobox-component
branch
2 participants
Description
This PR adds the Combobox and Command components to the Propel UI package.
Summary by CodeRabbit
New Features
Style