korbit-ai[bot] commented on code in PR #35122:
URL: https://github.com/apache/superset/pull/35122#discussion_r2363469150
##########
superset-frontend/src/components/Chart/useDrillDetailMenuItems/index.tsx:
##########
@@ -191,79 +172,110 @@ export const useDrillDetailMenuItems = ({
drillByDisabled = DISABLED_REASONS.NOT_SUPPORTED;
}
- const drillToDetailMenuItem: MenuItem = drillDisabled
- ? getDisabledMenuItem(
- <>
- {DRILL_TO_DETAIL}
- <MenuItemTooltip title={drillDisabled} />
- </>,
- 'drill-to-detail-disabled',
- props,
- )
+ const drillToDetailMenuItem: ItemType = drillDisabled
+ ? {
+ key: 'drill-to-detail-disabled',
+ disabled: true,
+ label: (
+ <div
+ css={css`
+ white-space: normal;
+ max-width: 160px;
+ `}
+ >
+ {DRILL_TO_DETAIL}
+ <MenuItemTooltip title={drillDisabled} />
+ </div>
+ ),
+ ...props,
+ }
: {
key: 'drill-to-detail',
- label: DRILL_TO_DETAIL,
onClick: openModal.bind(null, []),
- ...props,
+ label: DRILL_TO_DETAIL,
};
- const getMenuItemWithTruncation = useMenuItemWithTruncation();
-
- const drillToDetailByMenuItem: MenuItem = drillByDisabled
- ? getDisabledMenuItem(
- <>
- {DRILL_TO_DETAIL_BY}
- <MenuItemTooltip title={drillByDisabled} />
- </>,
- 'drill-to-detail-by-disabled',
- props,
- )
- : {
- key: key || 'drill-to-detail-by',
- label: DRILL_TO_DETAIL_BY,
- children: [
- ...filters.map((filter, i) => ({
- key: `drill-detail-filter-${i}`,
- label: getMenuItemWithTruncation({
- tooltipText: `${DRILL_TO_DETAIL_BY} ${filter.formattedVal}`,
- onClick: openModal.bind(null, [filter]),
+ const drillToDetailByMenuItem: ItemType | null = !isContextMenu
+ ? null
+ : drillByDisabled
+ ? {
+ key: 'drill-to-detail-by-disabled',
+ disabled: true,
+ label: (
+ <div
+ css={css`
+ white-space: normal;
+ max-width: 160px;
+ `}
+ >
+ {DRILL_TO_DETAIL_BY}
+ <MenuItemTooltip title={drillByDisabled} />
+ </div>
+ ),
+ ...props,
+ }
+ : {
+ key: key || 'drill-to-detail-by',
+ label: DRILL_TO_DETAIL_BY,
+ popupOffset: [0, submenuYOffset],
+ popupClassName: 'chart-context-submenu',
+ children: [
+ ...filters.map((filter, i) => ({
key: `drill-detail-filter-${i}`,
- children: (
- <>
- {`${DRILL_TO_DETAIL_BY} `}
- <StyledFilter stripHTML>{filter.formattedVal}</StyledFilter>
- </>
- ),
- }),
- })),
- filters.length > 1 && {
- key: 'drill-detail-filter-all',
- label: getMenuItemWithTruncation({
- tooltipText: `${DRILL_TO_DETAIL_BY} ${t('all')}`,
- onClick: openModal.bind(null, filters),
- key: 'drill-detail-filter-all',
- children: (
- <>
- {`${DRILL_TO_DETAIL_BY} `}
- <StyledFilter stripHTML={false}>{t('all')}</StyledFilter>
- </>
+ onClick: openModal.bind(null, [filter]),
+ label: (
+ <div
+ css={css`
+ max-width: 200px;
+ `}
+ >
+ <TruncatedMenuLabel
+ tooltipText={`${DRILL_TO_DETAIL_BY}
${filter.formattedVal}`}
+ aria-label={`${DRILL_TO_DETAIL_BY} ${filter.formattedVal}`}
+ >
+ <span
+ css={css`
+ display: inline;
+ `}
+ >
+ {DRILL_TO_DETAIL_BY}{' '}
+ <StyledFilter stripHTML>
+ {filter.formattedVal}
+ </StyledFilter>
+ </span>
+ </TruncatedMenuLabel>
+ </div>
),
- }),
- },
- ].filter(Boolean) as MenuItem[],
- onClick: openModal.bind(null, filters),
- forceSubmenuRender: true,
- popupOffset: [0, submenuYOffset],
- popupClassName: 'chart-context-submenu',
- ...props,
- };
- if (isContextMenu) {
- return {
- drillToDetailMenuItem,
- drillToDetailByMenuItem,
- };
+ })),
Review Comment:
### Unnecessary object recreation in filters.map <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The filters.map() creates a new menu item object for each filter on every
render, causing unnecessary re-renders and object allocations.
###### Why this matters
This leads to performance degradation when there are many filters, as React
will treat each menu item as a new object and re-render the entire submenu
unnecessarily.
###### Suggested change ∙ *Feature Preview*
Memoize the filter menu items using useMemo with filters as dependency, or
extract the mapping function to avoid inline object creation:
```typescript
const filterMenuItems = useMemo(
() => filters.map((filter, i) => ({
key: `drill-detail-filter-${i}`,
onClick: openModal.bind(null, [filter]),
label: (
// ... existing label JSX
),
})),
[filters, openModal]
);
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/5663f440-3896-4cf1-a58a-ab6987cc6f8a/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/5663f440-3896-4cf1-a58a-ab6987cc6f8a?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/5663f440-3896-4cf1-a58a-ab6987cc6f8a?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/5663f440-3896-4cf1-a58a-ab6987cc6f8a?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/5663f440-3896-4cf1-a58a-ab6987cc6f8a)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:5fd1575c-5d2f-4a19-9915-da1970ab258e -->
[](5fd1575c-5d2f-4a19-9915-da1970ab258e)
##########
superset-frontend/src/dashboard/components/SliceHeader/index.tsx:
##########
@@ -85,7 +85,7 @@ const ChartHeaderStyles = styled.div`
& > .header-title {
overflow: hidden;
text-overflow: ellipsis;
- max-width: 100%;
+ max-width: calc(100% - ${theme.sizeUnit * 4}px);
Review Comment:
### Hardcoded header controls width assumption <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The hardcoded subtraction of `theme.sizeUnit * 4` pixels may not accurately
reflect the actual width of the header controls, potentially causing text
truncation or overflow issues.
###### Why this matters
If the actual width of the header controls differs from the hardcoded 4 size
units, the title text may be unnecessarily truncated when controls are
narrower, or may overflow and overlap with controls when they are wider.
###### Suggested change ∙ *Feature Preview*
Calculate the actual width of the header controls dynamically using a ref
and ResizeObserver, or use CSS flexbox properties to let the browser handle the
space distribution automatically:
```css
& > .header-title {
overflow: hidden;
text-overflow: ellipsis;
flex: 1 1 0;
min-width: 0;
display: -webkit-box;
-webkit-line-clamp: 2;
-webkit-box-orient: vertical;
}
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/05eefb77-93fb-4dc5-b7eb-4acdcac52a9c/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/05eefb77-93fb-4dc5-b7eb-4acdcac52a9c?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/05eefb77-93fb-4dc5-b7eb-4acdcac52a9c?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/05eefb77-93fb-4dc5-b7eb-4acdcac52a9c?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/05eefb77-93fb-4dc5-b7eb-4acdcac52a9c)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:6491489f-1d9b-453d-b199-a75f5822630d -->
[](6491489f-1d9b-453d-b199-a75f5822630d)
##########
superset-frontend/src/components/Chart/DrillBy/DrillBySubmenu.tsx:
##########
@@ -0,0 +1,342 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import {
+ CSSProperties,
+ ReactNode,
+ useCallback,
+ useEffect,
+ useMemo,
+ useRef,
+ useState,
+} from 'react';
+import {
+ BaseFormData,
+ Behavior,
+ Column,
+ ContextMenuFilters,
+ css,
+ ensureIsArray,
+ getChartMetadataRegistry,
+ t,
+ useTheme,
+} from '@superset-ui/core';
+import {
+ Constants,
+ Input,
+ Loading,
+ Popover,
+ Icons,
+} from '@superset-ui/core/components';
+import { debounce } from 'lodash';
+import { FixedSizeList as List } from 'react-window';
+import { InputRef } from 'antd';
+import { MenuItemTooltip } from '../DisabledMenuItemTooltip';
+import { VirtualizedMenuItem } from '../MenuItemWithTruncation';
+import { Dataset } from '../types';
+
+const SUBMENU_HEIGHT = 200;
+const SHOW_COLUMNS_SEARCH_THRESHOLD = 10;
+
+export interface DrillBySubmenuProps {
+ drillByConfig?: ContextMenuFilters['drillBy'];
+ formData: BaseFormData & { [key: string]: any };
+ onSelection?: (...args: any) => void;
+ onClick?: (event: MouseEvent) => void;
+ onCloseMenu?: () => void;
+ openNewModal?: boolean;
+ excludedColumns?: Column[];
+ onDrillBy?: (column: Column, dataset: Dataset) => void;
+ dataset?: Dataset;
+ isLoadingDataset?: boolean;
+}
+
+export const DrillBySubmenu = ({
+ drillByConfig,
+ formData,
+ onSelection = () => {},
+ onClick = () => {},
+ onCloseMenu = () => {},
+ openNewModal = true,
+ excludedColumns,
+ onDrillBy,
+ dataset,
+ isLoadingDataset = false,
+ ...rest
+}: DrillBySubmenuProps) => {
Review Comment:
### Component has too many responsibilities <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The component is handling too many responsibilities including search,
filtering, UI state management, and event handling, violating the Single
Responsibility Principle.
###### Why this matters
Large components with multiple responsibilities are harder to maintain,
test, and reuse. This can lead to increased complexity and technical debt over
time.
###### Suggested change ∙ *Feature Preview*
Extract functionality into smaller, focused components:
```typescript
const SearchInput = ({ onSearch, showSearch, ...props }) => { /* ... */ };
const ColumnsList = ({ columns, onSelect }) => { /* ... */ };
const DrillByMenu = ({ isDisabled, tooltip, ...props }) => { /* ... */ };
export const DrillBySubmenu = (props: DrillBySubmenuProps) => {
// Core logic for coordinating between components
return (
<DrillByMenu {...menuProps}>
<SearchInput {...searchProps} />
<ColumnsList {...columnsProps} />
</DrillByMenu>
);
};
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/5808afb8-54c7-464e-a208-9f8b7ac3c78c/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/5808afb8-54c7-464e-a208-9f8b7ac3c78c?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/5808afb8-54c7-464e-a208-9f8b7ac3c78c?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/5808afb8-54c7-464e-a208-9f8b7ac3c78c?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/5808afb8-54c7-464e-a208-9f8b7ac3c78c)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:ac76972c-4f5b-486b-9665-d44026473b00 -->
[](ac76972c-4f5b-486b-9665-d44026473b00)
##########
superset-frontend/src/components/Chart/DrillBy/DrillBySubmenu.tsx:
##########
@@ -0,0 +1,342 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import {
+ CSSProperties,
+ ReactNode,
+ useCallback,
+ useEffect,
+ useMemo,
+ useRef,
+ useState,
+} from 'react';
+import {
+ BaseFormData,
+ Behavior,
+ Column,
+ ContextMenuFilters,
+ css,
+ ensureIsArray,
+ getChartMetadataRegistry,
+ t,
+ useTheme,
+} from '@superset-ui/core';
+import {
+ Constants,
+ Input,
+ Loading,
+ Popover,
+ Icons,
+} from '@superset-ui/core/components';
+import { debounce } from 'lodash';
+import { FixedSizeList as List } from 'react-window';
+import { InputRef } from 'antd';
+import { MenuItemTooltip } from '../DisabledMenuItemTooltip';
+import { VirtualizedMenuItem } from '../MenuItemWithTruncation';
+import { Dataset } from '../types';
+
+const SUBMENU_HEIGHT = 200;
+const SHOW_COLUMNS_SEARCH_THRESHOLD = 10;
+
+export interface DrillBySubmenuProps {
+ drillByConfig?: ContextMenuFilters['drillBy'];
+ formData: BaseFormData & { [key: string]: any };
+ onSelection?: (...args: any) => void;
+ onClick?: (event: MouseEvent) => void;
+ onCloseMenu?: () => void;
+ openNewModal?: boolean;
+ excludedColumns?: Column[];
+ onDrillBy?: (column: Column, dataset: Dataset) => void;
+ dataset?: Dataset;
+ isLoadingDataset?: boolean;
+}
+
+export const DrillBySubmenu = ({
+ drillByConfig,
+ formData,
+ onSelection = () => {},
+ onClick = () => {},
+ onCloseMenu = () => {},
+ openNewModal = true,
+ excludedColumns,
+ onDrillBy,
+ dataset,
+ isLoadingDataset = false,
+ ...rest
+}: DrillBySubmenuProps) => {
+ const theme = useTheme();
+ const [searchInput, setSearchInput] = useState('');
+ const [debouncedSearchInput, setDebouncedSearchInput] = useState('');
+ const [popoverOpen, setPopoverOpen] = useState(false);
+ const ref = useRef<InputRef>(null);
+ const menuItemRef = useRef<HTMLDivElement>(null);
+
+ const columns = useMemo(
+ () => (dataset ? ensureIsArray(dataset.drillable_columns) : []),
+ [dataset],
+ );
+ const showSearch = columns.length > SHOW_COLUMNS_SEARCH_THRESHOLD;
+
+ const handleSelection = useCallback(
+ (event, column) => {
+ onClick(event as MouseEvent);
+ onSelection(column, drillByConfig);
+ if (openNewModal && onDrillBy && dataset) {
+ onDrillBy(column, dataset);
+ }
+ setPopoverOpen(false);
+ onCloseMenu();
+ },
+ [
+ drillByConfig,
+ onClick,
+ onSelection,
+ openNewModal,
+ onDrillBy,
+ dataset,
+ onCloseMenu,
+ ],
+ );
+
+ useEffect(() => {
+ let timeoutId: NodeJS.Timeout;
+ if (popoverOpen) {
+ // Small delay to ensure popover is rendered
+ timeoutId = setTimeout(() => {
+ ref.current?.input?.focus({ preventScroll: true });
+ }, 100);
+ } else {
+ // Reset search input when menu is closed
+ setSearchInput('');
+ setDebouncedSearchInput('');
+ }
+ return () => {
+ if (timeoutId) clearTimeout(timeoutId);
+ };
+ }, [popoverOpen]);
+
+ const hasDrillBy = drillByConfig?.groupbyFieldName;
+
+ const handlesDimensionContextMenu = useMemo(
+ () =>
+ getChartMetadataRegistry()
+ .get(formData.viz_type)
+ ?.behaviors.find(behavior => behavior === Behavior.DrillBy),
+ [formData.viz_type],
+ );
+
+ const debouncedSetSearchInput = useMemo(
+ () =>
+ debounce((value: string) => {
+ setDebouncedSearchInput(value);
+ }, Constants.FAST_DEBOUNCE),
+ [],
+ );
Review Comment:
### Uncancelled debounce on unmount <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
Debounced search handler is created with lodash debounce but there is no
cleanup on unmount.
###### Why this matters
If the component unmounts while a debounced timer is pending, the timer may
fire and call state updates on an unmounted component, potentially leading to
memory leaks or React state warnings.
###### Suggested change ∙ *Feature Preview*
Add a cleanup effect to cancel the debounced function when the component
unmounts, for example:
```tsx
useEffect(() => {
return () => {
(debouncedSetSearchInput as any).cancel?.();
};
}, [debouncedSetSearchInput]);
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a6700ad1-562d-4855-96ef-2d1f2deb5669/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a6700ad1-562d-4855-96ef-2d1f2deb5669?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a6700ad1-562d-4855-96ef-2d1f2deb5669?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a6700ad1-562d-4855-96ef-2d1f2deb5669?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a6700ad1-562d-4855-96ef-2d1f2deb5669)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:4e038187-3a3c-4f54-9885-a9aedd13319a -->
[](4e038187-3a3c-4f54-9885-a9aedd13319a)
##########
superset-frontend/src/components/Chart/useDrillDetailMenuItems/index.tsx:
##########
@@ -191,79 +172,110 @@ export const useDrillDetailMenuItems = ({
drillByDisabled = DISABLED_REASONS.NOT_SUPPORTED;
}
- const drillToDetailMenuItem: MenuItem = drillDisabled
- ? getDisabledMenuItem(
- <>
- {DRILL_TO_DETAIL}
- <MenuItemTooltip title={drillDisabled} />
- </>,
- 'drill-to-detail-disabled',
- props,
- )
+ const drillToDetailMenuItem: ItemType = drillDisabled
+ ? {
+ key: 'drill-to-detail-disabled',
+ disabled: true,
+ label: (
+ <div
+ css={css`
+ white-space: normal;
+ max-width: 160px;
+ `}
+ >
+ {DRILL_TO_DETAIL}
+ <MenuItemTooltip title={drillDisabled} />
+ </div>
+ ),
+ ...props,
+ }
: {
key: 'drill-to-detail',
- label: DRILL_TO_DETAIL,
onClick: openModal.bind(null, []),
- ...props,
+ label: DRILL_TO_DETAIL,
};
- const getMenuItemWithTruncation = useMenuItemWithTruncation();
-
- const drillToDetailByMenuItem: MenuItem = drillByDisabled
- ? getDisabledMenuItem(
- <>
- {DRILL_TO_DETAIL_BY}
- <MenuItemTooltip title={drillByDisabled} />
- </>,
- 'drill-to-detail-by-disabled',
- props,
- )
- : {
- key: key || 'drill-to-detail-by',
- label: DRILL_TO_DETAIL_BY,
- children: [
- ...filters.map((filter, i) => ({
- key: `drill-detail-filter-${i}`,
- label: getMenuItemWithTruncation({
- tooltipText: `${DRILL_TO_DETAIL_BY} ${filter.formattedVal}`,
- onClick: openModal.bind(null, [filter]),
+ const drillToDetailByMenuItem: ItemType | null = !isContextMenu
+ ? null
+ : drillByDisabled
+ ? {
+ key: 'drill-to-detail-by-disabled',
+ disabled: true,
+ label: (
+ <div
+ css={css`
+ white-space: normal;
+ max-width: 160px;
+ `}
+ >
+ {DRILL_TO_DETAIL_BY}
+ <MenuItemTooltip title={drillByDisabled} />
+ </div>
+ ),
+ ...props,
+ }
+ : {
+ key: key || 'drill-to-detail-by',
+ label: DRILL_TO_DETAIL_BY,
+ popupOffset: [0, submenuYOffset],
+ popupClassName: 'chart-context-submenu',
+ children: [
+ ...filters.map((filter, i) => ({
key: `drill-detail-filter-${i}`,
- children: (
- <>
- {`${DRILL_TO_DETAIL_BY} `}
- <StyledFilter stripHTML>{filter.formattedVal}</StyledFilter>
- </>
- ),
- }),
- })),
- filters.length > 1 && {
- key: 'drill-detail-filter-all',
- label: getMenuItemWithTruncation({
- tooltipText: `${DRILL_TO_DETAIL_BY} ${t('all')}`,
- onClick: openModal.bind(null, filters),
- key: 'drill-detail-filter-all',
- children: (
- <>
- {`${DRILL_TO_DETAIL_BY} `}
- <StyledFilter stripHTML={false}>{t('all')}</StyledFilter>
- </>
+ onClick: openModal.bind(null, [filter]),
+ label: (
+ <div
+ css={css`
+ max-width: 200px;
+ `}
+ >
+ <TruncatedMenuLabel
+ tooltipText={`${DRILL_TO_DETAIL_BY}
${filter.formattedVal}`}
+ aria-label={`${DRILL_TO_DETAIL_BY} ${filter.formattedVal}`}
+ >
+ <span
+ css={css`
+ display: inline;
+ `}
+ >
+ {DRILL_TO_DETAIL_BY}{' '}
+ <StyledFilter stripHTML>
+ {filter.formattedVal}
+ </StyledFilter>
+ </span>
+ </TruncatedMenuLabel>
+ </div>
),
- }),
- },
- ].filter(Boolean) as MenuItem[],
- onClick: openModal.bind(null, filters),
- forceSubmenuRender: true,
- popupOffset: [0, submenuYOffset],
- popupClassName: 'chart-context-submenu',
- ...props,
- };
- if (isContextMenu) {
- return {
- drillToDetailMenuItem,
- drillToDetailByMenuItem,
- };
+ })),
+ ...(filters.length > 1
+ ? [
+ {
+ key: 'drill-detail-filter-all',
+ onClick: openModal.bind(null, filters),
+ label: (
+ <div
+ aria-label={`${DRILL_TO_DETAIL_BY} ${t('all')}`}
+ css={css`
+ max-width: 200px;
+ `}
+ >
+ {`${DRILL_TO_DETAIL_BY} `}
+ <StyledFilter stripHTML={false}>
+ {t('all')}
+ </StyledFilter>
+ </div>
+ ),
+ },
+ ]
+ : []),
+ ],
+ ...props,
+ };
+
+ const menuItems: ItemType[] = [drillToDetailMenuItem];
+ if (drillToDetailByMenuItem) {
+ menuItems.push(drillToDetailByMenuItem);
}
- return {
- drillToDetailMenuItem,
- };
+
+ return menuItems;
Review Comment:
### Array recreation on every render <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
Creating and mutating a new array on every render instead of using a
memoized approach.
###### Why this matters
This causes the hook to return a new array reference on every call,
triggering unnecessary re-renders in components that depend on this menu items
array.
###### Suggested change ∙ *Feature Preview*
Memoize the final menu items array using useMemo:
```typescript
return useMemo(() => {
const items = [drillToDetailMenuItem];
if (drillToDetailByMenuItem) {
items.push(drillToDetailByMenuItem);
}
return items;
}, [drillToDetailMenuItem, drillToDetailByMenuItem]);
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a35e9e26-9ac7-4e2e-b6df-a0205dde3d51/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a35e9e26-9ac7-4e2e-b6df-a0205dde3d51?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a35e9e26-9ac7-4e2e-b6df-a0205dde3d51?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a35e9e26-9ac7-4e2e-b6df-a0205dde3d51?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a35e9e26-9ac7-4e2e-b6df-a0205dde3d51)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:6320eb51-7c6b-4787-ae77-bc36ee5675a0 -->
[](6320eb51-7c6b-4787-ae77-bc36ee5675a0)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
