Skip to content

chore(compass-context-menu): conditionally filter groups COMPASS-9645 #7158

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Aug 1, 2025
Merged

chore(compass-context-menu): conditionally filter groups COMPASS-9645 #7158

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
77747b8
fix(compass-context-menu): conditionally filter groups
gagik Jul 31, 2025
cd345db
fix: also handle empty arrays
gagik Jul 31, 2025
29adeb4
fix: use type guards on functions
gagik Aug 1, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions packages/compass-components/src/components/context-menu.spec.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,11 @@ import { render, screen, userEvent } from '@mongodb-js/testing-library-compass';
import { expect } from 'chai';
import sinon from 'sinon';
import { ContextMenuProvider } from '@mongodb-js/compass-context-menu';
import { useContextMenuItems, ContextMenu } from './context-menu';
import type { ContextMenuItem } from '@mongodb-js/compass-context-menu';
import {
useContextMenuItems,
ContextMenu,
type ContextMenuItem,
} from './context-menu';

describe('useContextMenuItems', function () {
const menuTestTriggerId = 'test-trigger';
Expand Down
29 changes: 23 additions & 6 deletions packages/compass-components/src/components/context-menu.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,22 +117,39 @@ export function ContextMenu({ menu }: ContextMenuWrapperProps) {
);
}

/** Registers context menu items - items that are `undefined` will get filtered. */
export function useContextMenuItems(
getItems: () => ContextMenuItem[],
getItems: () => (ContextMenuItem | undefined)[],
dependencies: React.DependencyList | undefined
): React.RefCallback<HTMLElement> {
// eslint-disable-next-line react-hooks/exhaustive-deps
const memoizedItems = useMemo(getItems, dependencies);
const memoizedItems = useMemo(
() => getItems().filter((item) => item !== undefined),
Copy link

Copilot AI Jul 31, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
() => getItems().filter((item) => item !== undefined),
() => getItems().filter((item): item is ContextMenuItem => item !== undefined),

Copilot uses AI. Check for mistakes.
Copy link
Contributor Author

@gagik gagik Jul 31, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sounds right in theory but doesn't actually do anything in this context

Copy link
Contributor Author

@gagik gagik Aug 1, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

// eslint-disable-next-line react-hooks/exhaustive-deps
dependencies
);
const contextMenu = useContextMenu();
return contextMenu.registerItems(memoizedItems);
}

/** Registers context menu groups - groups and items that are `undefined` will get filtered. */
export function useContextMenuGroups(
getGroups: () => ContextMenuItemGroup[],
getGroups: () => ((ContextMenuItem | undefined)[] | undefined)[],
dependencies: React.DependencyList | undefined
): React.RefCallback<HTMLElement> {
// eslint-disable-next-line react-hooks/exhaustive-deps
const memoizedGroups = useMemo(getGroups, dependencies);
const memoizedGroups: ContextMenuItem[][] = useMemo(
() => {
const groups = getGroups();
// Cleanup all undefined fields across items and groups which is used
// for conditional displaying of groups and items.
return groups
.filter(
(groupItems) => groupItems !== undefined && groupItems.length > 0
Copy link
Contributor

@kraenhansen kraenhansen Aug 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: When doing things like this (obj or undefined) I tend to rely on array objects being the truthy.

Suggested change
(groupItems) => groupItems !== undefined && groupItems.length > 0
(groupItems) => groupItems && groupItems.length > 0

Copy link
Contributor Author

@gagik gagik Aug 1, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree generally but this leads to a weird state
Screenshot 2025-08-01 at 10 13 47 AM
Screenshot 2025-08-01 at 10 14 11 AM

Copy link
Contributor Author

@gagik gagik Aug 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

because

undefined && true // undefined
undefined && false // undefined

)
.map((groupItems) => groupItems!.filter((item) => item !== undefined));
Copy link
Contributor

@kraenhansen kraenhansen Aug 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does #7158 (comment) apply to the previous line then? Would that make you able to avoid the not-null assert?

Copy link
Contributor Author

@gagik gagik Aug 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah actually the function type assertion does work. not sure why it didn't before when I was trying it

},
// eslint-disable-next-line react-hooks/exhaustive-deps
dependencies
);
const contextMenu = useContextMenu();
return contextMenu.registerItems(...memoizedGroups);
}
44 changes: 20 additions & 24 deletions packages/compass-components/src/components/document-list/element.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -501,24 +501,22 @@ export const HadronElement: React.FunctionComponent<{
// Add context menu hook for the field
const fieldContextMenuRef = useContextMenuItems(
() => [
...(onUpdateQuery
? [
{
label: isFieldInQuery(
onUpdateQuery
? {
label: isFieldInQuery(
getNestedKeyPathForElement(element),
element.generateObject()
)
? 'Remove from query'
: 'Add to query',
onAction: () => {
onUpdateQuery(
getNestedKeyPathForElement(element),
element.generateObject()
)
? 'Remove from query'
: 'Add to query',
onAction: () => {
onUpdateQuery(
getNestedKeyPathForElement(element),
element.generateObject()
);
},
);
},
]
: []),
}
: undefined,
{
label: 'Copy field & value',
onAction: () => {
Expand All @@ -527,16 +525,14 @@ export const HadronElement: React.FunctionComponent<{
);
},
},
...(type.value === 'String' && isValidUrl(value.value)
? [
{
label: 'Open URL in browser',
onAction: () => {
window.open(value.value, '_blank', 'noopener');
},
type.value === 'String' && isValidUrl(value.value)
? {
label: 'Open URL in browser',
onAction: () => {
window.open(value.value, '_blank', 'noopener');
},
]
: []),
}
: undefined,
],
[element, key.value, value.value, type.value, onUpdateQuery, isFieldInQuery]
);
Expand Down
32 changes: 14 additions & 18 deletions packages/compass-crud/src/components/crud-toolbar.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -217,26 +217,22 @@ const CrudToolbar: React.FunctionComponent<CrudToolbarProps> = ({
onCollapseAllClicked();
},
},
...(isImportExportEnabled
? [
{
label: 'Import JSON or CSV file',
onAction: () => {
insertDataHandler('import-file');
},
isImportExportEnabled
? {
label: 'Import JSON or CSV file',
onAction: () => {
insertDataHandler('import-file');
},
]
: []),
...(!readonly
? [
{
label: 'Insert document...',
onAction: () => {
insertDataHandler('insert-document');
},
}
: undefined,
!readonly
? {
label: 'Insert document...',
onAction: () => {
insertDataHandler('insert-document');
},
]
: []),
}
: undefined,
...(isImportExportEnabled
? [
{
Expand Down
29 changes: 14 additions & 15 deletions packages/compass-crud/src/components/use-document-item-context-menu.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,24 +15,23 @@ export function useDocumentItemContextMenu({
openInsertDocumentDialog,
}: UseDocumentItemContextMenuProps) {
const { expanded: isExpanded, editing: isEditing } = doc;

return useContextMenuGroups(
() => [
[
...(isEditable
? [
{
label: isEditing ? 'Cancel editing' : 'Edit document',
onAction: () => {
if (isEditing) {
doc.finishEditing();
} else {
doc.startEditing();
}
},
isEditable
? [
{
label: isEditing ? 'Cancel editing' : 'Edit document',
onAction: () => {
if (isEditing) {
doc.finishEditing();
} else {
doc.startEditing();
}
},
]
: []),
],
},
]
: undefined,
[
{
label: isExpanded ? 'Collapse all fields' : 'Expand all fields',
Expand Down
Loading