Open
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
📝 WalkthroughWalkthroughMigrates the Combobox from Ariakit/Radix to Base UI primitives with generic single/multiple typing; refactors context and component APIs, updates tests and docs, adds combobox demos and playground exports, adds Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ComboboxRoot
participant ComboboxContext
participant ComboboxInput
participant ComboboxContent
participant ComboboxItem
User->>ComboboxRoot: Mount with items, value/defaultValue
ComboboxRoot->>ComboboxContext: Initialize context (inputValue, value, hasItems, inputContainerRef)
ComboboxRoot->>ComboboxInput: Render Input (containerRef attached)
User->>ComboboxInput: Type/search
ComboboxInput->>ComboboxRoot: onInputValueChange
ComboboxRoot->>ComboboxContext: Update inputValue
ComboboxContent->>ComboboxItem: Render filtered items (uses getMatch)
User->>ComboboxItem: Click/select option
ComboboxItem->>ComboboxRoot: onValueChange
ComboboxRoot->>ComboboxContext: Update value/computedValue
ComboboxRoot->>ComboboxInput: Reflect selection
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/raystack/components/dropdown-menu/utils.ts (1)
3-14:⚠️ Potential issue | 🟠 MajorRuntime error when
valueis not a string.Widening
valuetoanywhile callingvalue?.toLowerCase()on Line 12 will throw a TypeError for non-string values (e.g., numbers, objects). The function should guard against non-string types before calling string methods.🐛 Proposed fix to handle non-string values
export const getMatch = ( value?: any, children?: ReactNode, search?: string ) => { if (!search?.length) return true; const childrenValue = getChildrenValue(children)?.toLowerCase(); + const stringValue = typeof value === 'string' ? value.toLowerCase() : String(value ?? '').toLowerCase(); return ( - value?.toLowerCase().includes(search.toLowerCase()) || + stringValue.includes(search.toLowerCase()) || childrenValue?.includes(search.toLowerCase()) ); };packages/raystack/components/combobox/__tests__/combobox.test.tsx (1)
349-360:⚠️ Potential issue | 🔴 CriticalWire
onOpenChangeprop toComboboxPrimitive.Root.The
onOpenChangecallback is destructured inComboboxRootbut never passed to the Base UI primitive, causing it to never be invoked. PassonOpenChangetoComboboxPrimitive.Rootalongside the other event handlers (onValueChange,onInputValueChange).
🤖 Fix all issues with AI agents
In `@apps/www/src/content/docs/components/combobox/demo.ts`:
- Around line 54-68: The demo includes a duplicated option label "Lemon" in the
multiple-selection Combobox example; edit the demo in
apps/www/src/content/docs/components/combobox/demo.ts and remove the extra
Combobox.Item that renders "Lemon" so there is only one <Combobox.Item> with the
label "Lemon" in the <Combobox.Content> list (leave the other Combobox.Item
entries unchanged).
In `@packages/raystack/components/combobox/combobox-root.tsx`:
- Around line 85-86: The computedValue assignment uses the nullish coalescing
operator which treats explicit null like undefined; update the logic that sets
computedValue (currently using providedValue and internalValue in
combobox-root.tsx) to only fall back to internalValue when providedValue is
strictly undefined (e.g., check providedValue === undefined), so a controlled
value of null is preserved and not replaced by internalValue; locate the
computedValue variable near the top of the component and change the fallback
condition accordingly.
🧹 Nitpick comments (1)
packages/raystack/components/combobox/combobox-input.tsx (1)
29-36: Consider avoiding the redundant type assertion.Line 34 casts
value as string[]even though Line 30 already checksArray.isArray(value). TypeScript should infer this within the conditional block. If the assertion is still needed due to closure capture, consider extracting the value first.♻️ Suggested refinement
chips={ multiple && Array.isArray(value) - ? value.map(val => ({ + ? value.map((val: string) => ({ label: val, onRemove: () => - onValueChange?.((value as string[])?.filter(v => v !== val)) + onValueChange?.(value.filter(v => v !== val)) })) : undefined }
Comment on lines
54
to
+68
| <Combobox multiple> | ||
| <Combobox.Input placeholder="Select fruits..." /> | ||
| <Combobox.Input placeholder="Select fruits" width={300} /> | ||
| <Combobox.Content> | ||
| <Combobox.Item>Apple</Combobox.Item> | ||
| <Combobox.Item>Banana</Combobox.Item> | ||
| <Combobox.Item>Grape</Combobox.Item> | ||
| <Combobox.Item>Orange</Combobox.Item> | ||
| <Combobox.Item>Pineapple</Combobox.Item> | ||
| <Combobox.Item>Mango</Combobox.Item> | ||
| <Combobox.Item>Pineapple</Combobox.Item> | ||
| <Combobox.Item>Strawberry</Combobox.Item> | ||
| <Combobox.Item>Watermelon</Combobox.Item> | ||
| <Combobox.Item>Kiwi</Combobox.Item> | ||
| <Combobox.Item>Lemon</Combobox.Item> | ||
| <Combobox.Item>Lime</Combobox.Item> | ||
| <Combobox.Item>Lemon</Combobox.Item> |
Contributor
There was a problem hiding this comment.
Remove the duplicate “Lemon” option in the multiple selection demo.
It’s likely an accidental duplicate and can confuse the example.
✂️ Proposed fix
<Combobox.Item>Kiwi</Combobox.Item>
<Combobox.Item>Lemon</Combobox.Item>
<Combobox.Item>Lime</Combobox.Item>
- <Combobox.Item>Lemon</Combobox.Item>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| <Combobox multiple> | |
| <Combobox.Input placeholder="Select fruits..." /> | |
| <Combobox.Input placeholder="Select fruits" width={300} /> | |
| <Combobox.Content> | |
| <Combobox.Item>Apple</Combobox.Item> | |
| <Combobox.Item>Banana</Combobox.Item> | |
| <Combobox.Item>Grape</Combobox.Item> | |
| <Combobox.Item>Orange</Combobox.Item> | |
| <Combobox.Item>Pineapple</Combobox.Item> | |
| <Combobox.Item>Mango</Combobox.Item> | |
| <Combobox.Item>Pineapple</Combobox.Item> | |
| <Combobox.Item>Strawberry</Combobox.Item> | |
| <Combobox.Item>Watermelon</Combobox.Item> | |
| <Combobox.Item>Kiwi</Combobox.Item> | |
| <Combobox.Item>Lemon</Combobox.Item> | |
| <Combobox.Item>Lime</Combobox.Item> | |
| <Combobox.Item>Lemon</Combobox.Item> | |
| <Combobox multiple> | |
| <Combobox.Input placeholder="Select fruits" width={300} /> | |
| <Combobox.Content> | |
| <Combobox.Item>Apple</Combobox.Item> | |
| <Combobox.Item>Banana</Combobox.Item> | |
| <Combobox.Item>Grape</Combobox.Item> | |
| <Combobox.Item>Orange</Combobox.Item> | |
| <Combobox.Item>Mango</Combobox.Item> | |
| <Combobox.Item>Pineapple</Combobox.Item> | |
| <Combobox.Item>Strawberry</Combobox.Item> | |
| <Combobox.Item>Watermelon</Combobox.Item> | |
| <Combobox.Item>Kiwi</Combobox.Item> | |
| <Combobox.Item>Lemon</Combobox.Item> | |
| <Combobox.Item>Lime</Combobox.Item> | |
| </Combobox.Content> | |
| </Combobox> |
🤖 Prompt for AI Agents
In `@apps/www/src/content/docs/components/combobox/demo.ts` around lines 54 - 68,
The demo includes a duplicated option label "Lemon" in the multiple-selection
Combobox example; edit the demo in
apps/www/src/content/docs/components/combobox/demo.ts and remove the extra
Combobox.Item that renders "Lemon" so there is only one <Combobox.Item> with the
label "Lemon" in the <Combobox.Content> list (leave the other Combobox.Item
entries unchanged).
Comment on lines
+85
to
86
| const computedValue = providedValue ?? internalValue; | ||
|
|
Contributor
There was a problem hiding this comment.
Respect explicit null values for controlled single-select.
Using ?? causes value={null} to fall back to internalValue, which can surface stale selections in context.
✅ Proposed fix
- const computedValue = providedValue ?? internalValue;
+ const computedValue =
+ providedValue !== undefined ? providedValue : internalValue;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const computedValue = providedValue ?? internalValue; | |
| const computedValue = | |
| providedValue !== undefined ? providedValue : internalValue; | |
🤖 Prompt for AI Agents
In `@packages/raystack/components/combobox/combobox-root.tsx` around lines 85 -
86, The computedValue assignment uses the nullish coalescing operator which
treats explicit null like undefined; update the logic that sets computedValue
(currently using providedValue and internalValue in combobox-root.tsx) to only
fall back to internalValue when providedValue is strictly undefined (e.g., check
providedValue === undefined), so a controlled value of null is preserved and not
replaced by internalValue; locate the computedValue variable near the top of the
component and change the fallback condition accordingly.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@packages/raystack/components/combobox/combobox-root.tsx`:
- Around line 67-94: The inputValue is always from internal useState('') and
ignores externally controlled inputValue/defaultInputValue props; update
ComboboxRoot to accept inputValue and defaultInputValue props, initialize the
internal state with defaultInputValue (e.g., useState(defaultInputValue ?? '')),
derive a computedInputValue = props.inputValue ?? internalInputValue, and update
handleInputValueChange to setInternalInputValue via
setInputValue/setInternalInputValue and call onInputValueChange; ensure the
context/consumers use computedInputValue (not raw internal state) so
programmatic changes to inputValue or defaultInputValue are reflected.
Comment on lines
+67
to
+94
| export const ComboboxRoot = <Value extends unknown | unknown[]>({ | ||
| multiple = false, | ||
| children, | ||
| value: providedValue, | ||
| defaultValue = multiple ? [] : undefined, | ||
| onValueChange, | ||
| inputValue: providedInputValue, | ||
| onInputValueChange, | ||
| defaultInputValue, | ||
| open: providedOpen, | ||
| defaultOpen = false, | ||
| onOpenChange, | ||
| value: providedValue, | ||
| defaultValue, | ||
| items, | ||
| ...props | ||
| }: ComboboxRootProps) => { | ||
| }: ComboboxRootProps<Value>) => { | ||
| const [inputValue, setInputValue] = useState(''); | ||
| const [internalValue, setInternalValue] = useState< | ||
| string | string[] | undefined | ||
| >(defaultValue); | ||
| const [internalInputValue, setInternalInputValue] = | ||
| useState(defaultInputValue); | ||
| const [internalOpen, setInternalOpen] = useState(defaultOpen); | ||
| Value | Value[] | null | undefined | ||
| >(defaultValue ?? null); | ||
| const inputContainerRef = useRef<HTMLDivElement>(null); | ||
|
|
||
| const inputRef = useRef<HTMLInputElement>(null); | ||
| const listRef = useRef<HTMLDivElement>(null); | ||
| const computedValue = providedValue ?? internalValue; | ||
|
|
||
| const value = providedValue ?? internalValue; | ||
| const inputValue = providedInputValue ?? internalInputValue; | ||
| const open = providedOpen ?? internalOpen; | ||
| const handleInputValueChange = useCallback( | ||
| ( | ||
| value: string, | ||
| eventDetails: ComboboxPrimitive.Root.ChangeEventDetails | ||
| ) => { | ||
| setInputValue(value); | ||
| onInputValueChange?.(value); | ||
| }, | ||
| [onInputValueChange] | ||
| ); |
Contributor
There was a problem hiding this comment.
Sync context inputValue with controlled/default props.
inputValue is always sourced from internal state initialized to '', so defaultInputValue and externally controlled inputValue aren’t reflected in context. This can desync filtering/label behavior when parents programmatically set the input value.
✅ Proposed fix
}: ComboboxRootProps<Value>) => {
- const [inputValue, setInputValue] = useState('');
+ const providedInputValue = props.inputValue;
+ const defaultInputValue = props.defaultInputValue;
+ const [inputValue, setInputValue] = useState(defaultInputValue ?? '');
const [internalValue, setInternalValue] = useState<
Value | Value[] | null | undefined
>(defaultValue ?? null);
const inputContainerRef = useRef<HTMLDivElement>(null);
+ const computedInputValue =
+ providedInputValue !== undefined ? providedInputValue : inputValue;
const handleInputValueChange = useCallback(
(
value: string,
eventDetails: ComboboxPrimitive.Root.ChangeEventDetails
) => {
- setInputValue(value);
+ if (providedInputValue === undefined) {
+ setInputValue(value);
+ }
onInputValueChange?.(value);
},
- [onInputValueChange]
+ [onInputValueChange, providedInputValue]
);
const contextValue = useMemo(
() => ({
multiple,
- inputValue,
+ inputValue: computedInputValue,
hasItems: !!items,
inputContainerRef,
value: computedValue,
onValueChange: handleValueChange
}),
- [multiple, inputValue, items, computedValue, handleValueChange]
+ [multiple, computedInputValue, items, computedValue, handleValueChange]
);Also applies to: 115-124
🤖 Prompt for AI Agents
In `@packages/raystack/components/combobox/combobox-root.tsx` around lines 67 -
94, The inputValue is always from internal useState('') and ignores externally
controlled inputValue/defaultInputValue props; update ComboboxRoot to accept
inputValue and defaultInputValue props, initialize the internal state with
defaultInputValue (e.g., useState(defaultInputValue ?? '')), derive a
computedInputValue = props.inputValue ?? internalInputValue, and update
handleInputValueChange to setInternalInputValue via
setInputValue/setInternalInputValue and call onInputValueChange; ensure the
context/consumers use computedInputValue (not raw internal state) so
programmatic changes to inputValue or defaultInputValue are reflected.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
[Provide a brief description of the changes in this PR]
Type of Change
How Has This Been Tested?
[Describe the tests that you ran to verify your changes]
Checklist:
Screenshots (if appropriate):
[Add screenshots here]
Related Issues
[Link any related issues here using #issue-number]
Summary by CodeRabbit
New Features
Documentation