chore(compass-context-menu): conditionally filter groups COMPASS-9645 #7158
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR modifies context menu hooks to support conditional rendering by allowing undefined groups and items, eliminating the need to pass empty arrays that would create groups without items.
- Updates
useContextMenuItemsanduseContextMenuGroupsto accept and filter outundefinedvalues - Refactors conditional menu item logic to return
undefinedinstead of using spread operators with empty arrays - Adds proper TypeScript types and filtering logic to handle optional menu items and groups
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| packages/compass-components/src/components/context-menu.tsx | Core implementation of filtering logic for undefined menu items and groups |
| packages/compass-crud/src/components/use-document-item-context-menu.tsx | Simplified conditional menu item rendering using undefined instead of empty arrays |
| packages/compass-crud/src/components/crud-toolbar.tsx | Updated toolbar context menu to use undefined for conditional items |
| packages/compass-components/src/components/document-list/element.tsx | Refactored field context menu to return undefined for conditional items |
| packages/compass-components/src/components/context-menu.spec.tsx | Updated imports to use type imports for better TypeScript practices |
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| const memoizedItems = useMemo(getItems, dependencies); | ||
| const memoizedItems = useMemo( | ||
| () => getItems().filter((item) => item !== undefined), |
There was a problem hiding this comment.
The filtering logic should use a type guard to ensure type safety. Consider using (item): item is ContextMenuItem => item !== undefined instead of just item !== undefined.
| () => getItems().filter((item) => item !== undefined), | |
| () => getItems().filter((item): item is ContextMenuItem => item !== undefined), |
There was a problem hiding this comment.
sounds right in theory but doesn't actually do anything in this context
There was a problem hiding this comment.
actually this is useful for the other case where I'm also checking for array size (more relevant for the groupItems where I end up using a type assertion)
gagik
changed the title
gagik
added
the
no-title-validation
| // for conditional displaying of groups and items. | ||
| return groups | ||
| .filter( | ||
| (groupItems) => groupItems !== undefined && groupItems.length > 0 |
There was a problem hiding this comment.
Nit: When doing things like this (obj or undefined) I tend to rely on array objects being the truthy.
| (groupItems) => groupItems !== undefined && groupItems.length > 0 | |
| (groupItems) => groupItems && groupItems.length > 0 |
There was a problem hiding this comment.
Agree generally but this leads to a weird state
There was a problem hiding this comment.
because
undefined && true // undefined
undefined && false // undefined
| .filter( | ||
| (groupItems) => groupItems !== undefined && groupItems.length > 0 | ||
| ) | ||
| .map((groupItems) => groupItems!.filter((item) => item !== undefined)); |
There was a problem hiding this comment.
Does #7158 (comment) apply to the previous line then? Would that make you able to avoid the not-null assert?
There was a problem hiding this comment.
yeah actually the function type assertion does work. not sure why it didn't before when I was trying it
gagik
changed the title
Changes the hooks to support passing
undefinedgroups and items for cleaner conditional rendering and to avoid cases where we would end up passing[]as a group (thus leading to groups without any items).