feat(CommandMenu): sort menu items on top when opened #724
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
🦋 Changeset detectedLatest commit: 6d0ff42 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
📦 NPM canary releaseDeployed canary version 0.0.0-canary-1b4243f. |
There was a problem hiding this comment.
Pull Request Overview
This PR adds functionality to sort selected menu items to the top when the CommandMenu is opened in multiple selection mode. The implementation prevents content shifting during user interaction by capturing the initial selected state and using it consistently for sorting.
Key changes:
- Added initial selected keys tracking to maintain stable sorting during user interaction
- Implemented sorting logic that places selected items first in both filtered and unfiltered states
- Added comprehensive test coverage for the new sorting behavior
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| CommandMenu.tsx | Core implementation of selected items sorting logic with initial state tracking |
| CommandMenu.test.tsx | Comprehensive test suite covering sorting behavior in various scenarios |
| CommandMenu.stories.tsx | Updated story to demonstrate the new sorting feature |
| new-snails-raise.md | Changeset documentation for the feature |
| initialSelectedKeysRef.current = undefined; | ||
| } | ||
| } | ||
| }, [selectedKeys, defaultSelectedKeys]); // Depend on both props for initial setup |
There was a problem hiding this comment.
The useMemo dependency array includes selectedKeys and defaultSelectedKeys but the code inside references ariaSelectedKeys and ariaDefaultSelectedKeys. This mismatch could cause the memo to not update when the actual variables change, or update unnecessarily when unrelated variables change.
| }, [selectedKeys, defaultSelectedKeys]); // Depend on both props for initial setup | |
| }, [ariaSelectedKeys, ariaDefaultSelectedKeys]); // Depend on both props for initial setup |
Copilot uses AI. Check for mistakes.
|
|
||
| // Initialize initial selected keys only once on mount | ||
| // This captures the initial state and prevents sorting changes during user interaction | ||
| React.useMemo(() => { |
There was a problem hiding this comment.
Using useMemo for side effects (setting refs) is an anti-pattern. Consider using useEffect instead, as useMemo should only be used for expensive computations that return a value.
| React.useMemo(() => { | |
| React.useEffect(() => { |
Copilot uses AI. Check for mistakes.
| const sections: any[] = []; | ||
| const items: any[] = []; | ||
|
|
||
| [...items].forEach((item) => { | ||
| if (item.type === 'section') { | ||
| const filteredChildren = filterNodes(item.childNodes); | ||
| if (filteredChildren.length) { | ||
| result.push({ | ||
| ...item, | ||
| childNodes: filteredChildren, | ||
| nodeList.forEach((node) => { | ||
| if (node.type === 'section') { | ||
| // Process section children recursively | ||
| const sortedChildren = sortWithSelectedFirst( | ||
| Array.from(node.childNodes), | ||
| ); | ||
| sections.push({ | ||
| ...node, | ||
| childNodes: sortedChildren, | ||
| }); | ||
| } else { | ||
| items.push(node); | ||
| } | ||
| } else { | ||
| const text = item.textValue ?? String(item.rendered ?? ''); | ||
| if (enhancedFilter(text, term, item.props)) { | ||
| result.push(item); | ||
| } | ||
| } | ||
| }); | ||
| }); | ||
|
|
||
| return result; | ||
| }; | ||
| // Sort items and combine with sections | ||
| const sortedItems = sortWithSelectedFirst(items); | ||
| return [...sections, ...sortedItems]; | ||
| }; | ||
|
|
||
| return filterNodes(collectionItems); | ||
| }, [collectionItems, searchValue, enhancedFilter]); | ||
| resultItems = processNodes(collectionItems); | ||
| } | ||
|
|
||
| // Sort items at the root level with selected ones first (for items not in sections) | ||
| const sections: any[] = []; | ||
| const items: any[] = []; | ||
|
|
||
| resultItems.forEach((item) => { | ||
| if (item.type === 'section') { | ||
| sections.push(item); | ||
| } else { | ||
| items.push(item); | ||
| } | ||
| }); | ||
|
|
||
| const sortedItems = sortWithSelectedFirst(items); | ||
| return [...sections, ...sortedItems]; |
There was a problem hiding this comment.
This sorting logic is duplicated - the same separation of sections and items, followed by sorting, is performed both inside the processNodes function (lines 383-407) and again at the end (lines 410-423). This duplication could lead to maintenance issues and unnecessary processing.
| const sections: any[] = []; | |
| const items: any[] = []; | |
| [...items].forEach((item) => { | |
| if (item.type === 'section') { | |
| const filteredChildren = filterNodes(item.childNodes); | |
| if (filteredChildren.length) { | |
| result.push({ | |
| ...item, | |
| childNodes: filteredChildren, | |
| nodeList.forEach((node) => { | |
| if (node.type === 'section') { | |
| // Process section children recursively | |
| const sortedChildren = sortWithSelectedFirst( | |
| Array.from(node.childNodes), | |
| ); | |
| sections.push({ | |
| ...node, | |
| childNodes: sortedChildren, | |
| }); | |
| } else { | |
| items.push(node); | |
| } | |
| } else { | |
| const text = item.textValue ?? String(item.rendered ?? ''); | |
| if (enhancedFilter(text, term, item.props)) { | |
| result.push(item); | |
| } | |
| } | |
| }); | |
| }); | |
| return result; | |
| }; | |
| // Sort items and combine with sections | |
| const sortedItems = sortWithSelectedFirst(items); | |
| return [...sections, ...sortedItems]; | |
| }; | |
| return filterNodes(collectionItems); | |
| }, [collectionItems, searchValue, enhancedFilter]); | |
| resultItems = processNodes(collectionItems); | |
| } | |
| // Sort items at the root level with selected ones first (for items not in sections) | |
| const sections: any[] = []; | |
| const items: any[] = []; | |
| resultItems.forEach((item) => { | |
| if (item.type === 'section') { | |
| sections.push(item); | |
| } else { | |
| items.push(item); | |
| } | |
| }); | |
| const sortedItems = sortWithSelectedFirst(items); | |
| return [...sections, ...sortedItems]; | |
| return sortSectionsAndItems(nodeList, sortWithSelectedFirst); | |
| }; | |
| resultItems = processNodes(collectionItems); | |
| } | |
| // Sort items at the root level with selected ones first (for items not in sections) | |
| return sortSectionsAndItems(resultItems, sortWithSelectedFirst); |
Copilot uses AI. Check for mistakes.
| const unselectedItems: any[] = []; | ||
|
|
||
| items.forEach((item) => { | ||
| if (initialSelectedKeysRef.current!.has(item.key)) { |
There was a problem hiding this comment.
Using non-null assertion operator (!) is risky here. Although there's a check above, it's safer to use optional chaining or a more explicit null check to prevent potential runtime errors.
| if (initialSelectedKeysRef.current!.has(item.key)) { | |
| if (initialSelectedKeysRef.current?.has(item.key)) { |
Copilot uses AI. Check for mistakes.
🏋️ Size limit report
Click here if you want to find out what is changed in this build |
🧪 Storybook is successfully deployed!
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Bug: Incorrect Hook Usage Causes Stale Selection Data
The React.useMemo hook is misused for side effects by setting initialSelectedKeysRef. This should be useEffect. The useMemo also has an inconsistent dependency array [selectedKeys, defaultSelectedKeys] because its logic only runs once due to the hasInitializedRef.current guard. This prevents initialSelectedKeysRef from updating when selectedKeys or defaultSelectedKeys change, leading to stale selection data and incorrect sorting in multiple selection mode.
src/components/actions/CommandMenu/CommandMenu.tsx#L298-L311
cube-ui-kit/src/components/actions/CommandMenu/CommandMenu.tsx
Lines 298 to 311 in 6d0ff42
Was this report helpful? Give feedback by reacting with 👍 or 👎
No description provided.