feat(ui): Add CommandList compound component#9
Merged
Conversation
Data-source agnostic command palette supporting local filtering, server-side search, and hybrid modes. Includes keyboard navigation, grouping, fuzzy match highlight rendering, and virtualization support.
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds a new CommandList compound component and full supporting surface (styles, types, hooks, contexts, store, stories, and tests) implementing a searchable, groupable, highlight-capable command palette with keyboard navigation and optional virtualization. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Input as "Input Component"
participant State as "useCommandListState"
participant Store as "CommandListStore"
participant Results as "Results Component"
participant Item as "Item Component"
User->>Input: type query
Input->>State: onQueryChange(query)
State->>State: filter → score → sort → group → flatten
State->>Store: setQuery(query)\nsetItems(processed)
Store->>Store: update snapshot\nreset activeIndex
Store-->>Input: notify subscribers
Store-->>Results: notify subscribers
Results->>Store: getSnapshot()
Results->>Results: render groups/items (virtualize if enabled)
Results->>Item: renderItem(item, meta)
User->>Item: navigate / click / press Enter
Item->>Store: setActiveIndex / setActiveKey
Item->>State: actions.invoke(key)
State->>State: call onAction callback
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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}
There was a problem hiding this comment.
Actionable comments posted: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/base-ui/src/components/typography/Typography.module.css (1)
229-259:⚠️ Potential issue | 🟡 MinorKeep the
lghotkey badges square too.
min-widthwas added for the base andsmvariants, butdata-ov-size='lg'still only increasesmin-height. Single-key large hotkeys will render taller than they are wide.💡 Suggested fix
.Hotkey[data-ov-size='lg'] { min-height: calc(var(--ov-size-typography-hotkey-min-height) + 2px); + min-width: calc(var(--ov-size-typography-hotkey-min-height) + 2px); padding: calc(var(--ov-size-typography-hotkey-padding-block) + 1px) calc(var(--ov-size-typography-hotkey-padding-inline) + 1px); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/base-ui/src/components/typography/Typography.module.css` around lines 229 - 259, The .Hotkey[data-ov-size='lg'] rule only increases min-height so large hotkey badges become taller than wide; update that selector to also increase min-width (e.g., set min-width: calc(var(--ov-size-typography-hotkey-min-height) + 2px)) and mirror the padding adjustments already present so lg badges remain square; modify the .Hotkey[data-ov-size='lg'] block to adjust min-width and keep padding changes consistent with the min-height change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/base-ui/src/components/command-list/CommandList.module.css`:
- Around line 141-145: The CSS rule .Item[data-ov-disabled='true'] contains a
redundant cursor: default because pointer-events: none prevents the cursor style
from applying; remove the cursor: default declaration from the
.Item[data-ov-disabled='true'] rule to clean up the stylesheet while keeping
opacity and pointer-events intact.
In `@packages/base-ui/src/components/command-list/CommandList.stories.tsx`:
- Around line 519-521: The three inline buttons that call setQuery(''),
setQuery('git'), and setQuery('save') should include an explicit type attribute
to avoid being treated as form submitters; update each <button> element in
CommandList.stories (the buttons invoking setQuery) to add type="button" so they
behave purely as controls and not as form submit buttons.
- Around line 353-355: The three interactive controls in CommandList.stories.tsx
(the buttons that call setLoading and setItems and the one using
allCommands.slice) are missing explicit types and default to type="submit";
update each <button> that triggers setLoading/setItems to include type="button"
so they do not trigger form submission (i.e., add type="button" to the Show
Loading, Show Empty, and Show Results buttons).
- Around line 142-180: useDebouncedSearch leaves a pending timeout when the
component unmounts; add a useEffect in useDebouncedSearch that returns a cleanup
function which clears the timerRef (clearTimeout(timerRef.current) and set
timerRef.current = undefined) to avoid the timeout invoking state setters after
unmount. Keep the cleanup independent of the search callback and reference
timerRef and ReturnType<typeof setTimeout> as currently defined.
In `@packages/base-ui/src/components/command-list/CommandList.test.tsx`:
- Around line 331-337: The test in CommandList.test.tsx is using
screen.getAllByRole('presentation') to locate group headers which is fragile;
replace that selector with a more robust one (e.g., add/test for a stable
attribute like data-testid on the group header component or query by the exact
header text via screen.getByText) and then assert their DOM order
deterministically (for example, collect headers via
screen.getAllByTestId(/group-header-/) or
getByText('View')/getByText('File')/getByText('Search') and compare their
positions). Update the variable named headers and the three expect assertions to
use the new selector and order check so the test no longer depends on
role="presentation".
- Around line 373-394: The test "skips disabled items in keyboard navigation"
conflates two disabling mechanisms; update tests to cover each independently by
creating one test that marks an item disabled via the item object (e.g.,
TestCommand with disabled: true) and renders TestCommandList without
disabledKeys, and a second test that uses a plain items array and passes
disabledKeys={['b']} to TestCommandList to assert keyboard navigation skips the
disabled option; ensure both tests use the same assertion (checking options[2]
has data-ov-active="true") and keep the existing user.keyboard('{ArrowDown}')
flow and screen.getAllByRole('option') lookup.
In `@packages/base-ui/src/components/command-list/CommandList.tsx`:
- Around line 291-293: The code uses useSyncExternalStore(store.subscribe,
store.getSnapshot, store.getSnapshot) to subscribe for updates but then reads
data directly via store.getItems() and store.getItemState(), with snapshot
unused; add a short clarifying comment above the snapshot line explaining that
useSyncExternalStore is used solely to trigger re-renders on store changes
(hence the unused snapshot variable), while the actual item data is retrieved
from store.getItems() / store.getItemState(), and keep the void snapshot line to
avoid lint/unused-variable errors.
- Around line 153-165: The code recreates a new Map on every render (map and
itemsMapRef.current) causing unnecessary allocations; replace the manual
recreation with a memoized map using useMemo so the Map is only rebuilt when
items or itemKey change, then use that memoized Map in renderItemFn (referencing
itemsMapRef or replace itemsMapRef.current usage with the memoized map) and keep
renderItem in the renderItemFn dependency array; update references to
itemsMapRef, map, renderItemFn, items, and itemKey accordingly.
In `@packages/base-ui/src/components/command-list/hooks/useCommandListFocus.ts`:
- Around line 48-60: ArrowUp/ArrowDown compute an absolute index with
findNextEnabled but then call actions.moveActive with a delta; replace that
indirection by calling store.setActiveIndex(targetIndex) directly (use the
computed next/prev values and snapshot.activeIndex only to compute target via
findNextEnabled) so ArrowDown/ArrowUp mirror Home/End behavior; update the cases
to call store.setActiveIndex(next) / store.setActiveIndex(prev) when next/prev
>= 0 and remove the delta conversion to actions.moveActive.
In `@packages/base-ui/src/components/command-list/hooks/useCommandListState.ts`:
- Around line 177-190: The local variable items in moveActive shadows the items
prop; rename the local binding (e.g., storeItems or availableItems) to avoid
confusion, update all uses inside moveActive (replace items.length with
storeItems.length), keep the same early-return when length is 0, and continue to
use store.getSnapshot() and store.setActiveIndex(nextIndex) as before so
behavior is unchanged.
- Around line 67-101: The loop building processedItems calls itemKey(item)
multiple times; cache the key once per item (e.g., const key = itemKey(item))
and use that key for the ProcessedItem fields (key, highlightsProp.get(key),
disabledKeys.has(key)) and any further uses; update the creation of
ProcessedItem in useCommandListState to reference this cached key so itemKey is
invoked only once per item.
In `@packages/base-ui/src/components/command-list/hooks/useCommandListStore.ts`:
- Around line 72-82: The setActiveKey callback leaves activeIndexRef stale when
passed null; update setActiveKey (the function using activeKeyRef,
activeIndexRef, itemsRef, and emit) so that when key === null you also reset
activeIndexRef.current (e.g., to -1 or null) before calling emit, otherwise when
key is non-null keep the existing logic that finds the index from
itemsRef.current; ensure the chosen sentinel value is used consistently wherever
activeIndexRef is read.
- Around line 120-134: The stored storeRef created via useRef currently captures
the callbacks (subscribe, getSnapshot, getItemState, setActiveIndex,
setActiveKey, setQuery, setItems, setLoading, getItems, getItemByIndex) only at
initial render which is intentional but can lead to stale references if any of
those callbacks later gain changing dependencies; add a concise comment above
the storeRef creation explaining that the store identity must remain stable
across renders, that all referenced callbacks are expected to be stable
(useCallback with stable deps), and that any future changes to those callbacks
must preserve stability or update this pattern to avoid stale closures.
- Around line 55-60: getItemState currently does O(n) lookups via
itemsRef.current.find(...) causing O(n²) renders; fix by adding a ref (e.g.,
keyToIndexMapRef) inside useCommandListStore that is rebuilt whenever items
change and maps item.key to the item or its disabled flag, then change
getItemState to read from keyToIndexMapRef.current for O(1) lookups; update any
code that mutates itemsRef (the logic that sets itemsRef.current) to also
rebuild keyToIndexMapRef.current and ensure activeKeyRef/current usage remains
unchanged so useCommandListItem calls are cheap.
---
Outside diff comments:
In `@packages/base-ui/src/components/typography/Typography.module.css`:
- Around line 229-259: The .Hotkey[data-ov-size='lg'] rule only increases
min-height so large hotkey badges become taller than wide; update that selector
to also increase min-width (e.g., set min-width:
calc(var(--ov-size-typography-hotkey-min-height) + 2px)) and mirror the padding
adjustments already present so lg badges remain square; modify the
.Hotkey[data-ov-size='lg'] block to adjust min-width and keep padding changes
consistent with the min-height change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 95100c16-802d-414e-8076-9631e5122d6d
📒 Files selected for processing (14)
packages/base-ui/src/components/command-list/CommandList.module.csspackages/base-ui/src/components/command-list/CommandList.stories.tsxpackages/base-ui/src/components/command-list/CommandList.test.tsxpackages/base-ui/src/components/command-list/CommandList.tsxpackages/base-ui/src/components/command-list/constants.tspackages/base-ui/src/components/command-list/context/CommandListContext.tsxpackages/base-ui/src/components/command-list/hooks/useCommandListFocus.tspackages/base-ui/src/components/command-list/hooks/useCommandListItem.tspackages/base-ui/src/components/command-list/hooks/useCommandListState.tspackages/base-ui/src/components/command-list/hooks/useCommandListStore.tspackages/base-ui/src/components/command-list/index.tspackages/base-ui/src/components/command-list/types.tspackages/base-ui/src/components/index.tspackages/base-ui/src/components/typography/Typography.module.css
- Remove redundant cursor:default from disabled item CSS (pointer-events:none suffices) - Add type="button" to story control buttons to prevent form submission - Add cleanup useEffect to useDebouncedSearch to clear timeout on unmount - Replace fragile role="presentation" selector in group order test with DOM position comparison - Split disabled items test into separate disabledKeys and item-object variants - Memoize items Map in Root to avoid per-render allocation - Add clarifying comment on unused snapshot in Results (triggers re-renders only) - Use store.setActiveIndex directly in ArrowUp/Down instead of delta indirection - Rename shadowed items variable in moveActive to storeItems - Cache itemKey() call per item in processedItems to avoid triple invocation - Add O(1) key→item map in store for getItemState lookups - Reset activeIndex to -1 when setActiveKey(null) called - Add stability comment on storeRef creation - Add min-width to Hotkey lg variant for consistent square badges
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/base-ui/src/components/command-list/CommandList.tsx`:
- Around line 301-306: The virtualization tuning props are being ignored because
useListVirtualizer is called with a hardcoded estimatedItemSize (36) and no
overscan; update the plumbing so that CommandListConfigContextValue and
useCommandListState include estimatedItemSize and overscan, expose them on the
config returned by useCommandListState (alongside config.virtualized), and then
call useListVirtualizer({ count: allItems.length, scrollRef, enabled:
config.virtualized, estimatedItemSize: config.estimatedItemSize, overscan:
config.overscan }) in CommandList.Root (the component invoking
useListVirtualizer) so the values passed into CommandList.Root actually control
the virtualizer.
- Around line 493-517: Normalize and clamp highlight ranges before
sorting/merging: iterate the incoming ranges array and for each HighlightRange
from ranges clamp start and end to the bounds 0 and text.length, ensure start <
end (skip or adjust invalid ranges), then proceed to create sorted and merged
arrays (the variables sorted, merged, ranges, HighlightRange, text are where to
apply this). This prevents negative or out-of-range start/end values from
producing empty or misplaced segments when building segments and using cursor.
In `@packages/base-ui/src/components/command-list/hooks/useCommandListState.ts`:
- Around line 117-124: The current logic treats groupOrder as both order and
filter and drops any groups not listed; change the construction of order so that
when groupOrder is provided it defines the initial ordering but then appends any
remaining keys from groupMap that are not present in groupOrder. In other words,
compute order using groupOrder as a prefix and then concatenate
[...groupMap.keys()] filtered to exclude keys already in groupOrder; keep the
subsequent .filter(k => groupMap.has(k))/.map(...) logic unchanged. This uses
the existing symbols groupOrder, groupMap, and order to preserve all groups
while honoring the requested ordering.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: c050a9cb-8a98-45e5-954f-20df4bbf29f7
📒 Files selected for processing (8)
packages/base-ui/src/components/command-list/CommandList.module.csspackages/base-ui/src/components/command-list/CommandList.stories.tsxpackages/base-ui/src/components/command-list/CommandList.test.tsxpackages/base-ui/src/components/command-list/CommandList.tsxpackages/base-ui/src/components/command-list/hooks/useCommandListFocus.tspackages/base-ui/src/components/command-list/hooks/useCommandListState.tspackages/base-ui/src/components/command-list/hooks/useCommandListStore.tspackages/base-ui/src/components/typography/Typography.module.css
Comment on lines
+301
to
+306
| const virtualizer = useListVirtualizer({ | ||
| count: allItems.length, | ||
| scrollRef, | ||
| enabled: config.virtualized, | ||
| estimatedItemSize: 36, | ||
| }); |
There was a problem hiding this comment.
Virtualization tuning props are effectively ignored.
Line 305 hardcodes estimatedItemSize: 36, and no overscan is passed. CommandList.Root accepts these tuning props (Lines 112-113), but they currently do not affect useListVirtualizer.
🔧 Proposed fix
const virtualizer = useListVirtualizer({
count: allItems.length,
scrollRef,
enabled: config.virtualized,
- estimatedItemSize: 36,
+ estimatedItemSize: config.estimatedItemSize,
+ overscan: config.overscan,
});You’ll also need to plumb estimatedItemSize and overscan through CommandListConfigContextValue and useCommandListState so config exposes them here.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/base-ui/src/components/command-list/CommandList.tsx` around lines
301 - 306, The virtualization tuning props are being ignored because
useListVirtualizer is called with a hardcoded estimatedItemSize (36) and no
overscan; update the plumbing so that CommandListConfigContextValue and
useCommandListState include estimatedItemSize and overscan, expose them on the
config returned by useCommandListState (alongside config.virtualized), and then
call useListVirtualizer({ count: allItems.length, scrollRef, enabled:
config.virtualized, estimatedItemSize: config.estimatedItemSize, overscan:
config.overscan }) in CommandList.Root (the component invoking
useListVirtualizer) so the values passed into CommandList.Root actually control
the virtualizer.
- Plumb estimatedItemSize and overscan through config to useListVirtualizer instead of hardcoding estimatedItemSize=36 - Clamp and validate highlight ranges in splitByRanges to handle negative or out-of-bounds start/end values - Append unlisted group keys after groupOrder entries so groups not in groupOrder are preserved rather than dropped
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/base-ui/src/components/command-list/hooks/useCommandListState.ts`:
- Around line 162-168: Replace the per-render allocation of a new Map by
memoizing it: compute the map via useMemo keyed on the dependencies that affect
its content (items and itemKey) instead of creating "map" directly each render,
then assign that memoized Map to itemsMapRef.current; update references to the
local "map" variable to use the memoized value so itemsMapRef and any closures
keep a stable, memoized Map and avoid GC churn.
- Around line 184-197: The moveActive function currently clamps the nextIndex
but must respect the store.config.loopFocus setting like findNextEnabled does;
update moveActive (which reads store.getSnapshot() and store.getItems()) to
compute nextIndex = snapshot.activeIndex + delta, return early if len === 0, and
if store.config.loopFocus is true wrap the index using idx = ((idx % len) + len)
% len before calling store.setActiveIndex(nextIndex), otherwise keep the
existing clamp-to-[0,len-1] behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 39118cd1-1ee7-4925-8648-e5cb2dbf6acc
📒 Files selected for processing (3)
packages/base-ui/src/components/command-list/CommandList.tsxpackages/base-ui/src/components/command-list/hooks/useCommandListState.tspackages/base-ui/src/components/command-list/types.ts
- Memoize items Map in useCommandListState to avoid per-render allocation - Respect loopFocus in moveActive so index wraps instead of clamping
This was referenced Mar 8, 2026
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.
Summary
CommandListcompound component — an IDE command palette supporting local filtering, server-side/external search, and hybrid modes<mark>elementsMdKeyboardCommandKey,BsShift, etc.) for consistent sizingmin-widthfor uniform square badgesTest plan
pnpm typecheckpassespnpm test— all 267 tests pass (35 new CommandList tests)Lists/CommandListstories render correctlySummary by CodeRabbit