korbit-ai[bot] commented on code in PR #35122:
URL: https://github.com/apache/superset/pull/35122#discussion_r2359629486
##########
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:
### Inefficient CSS calc with runtime multiplication <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
CSS calc() function is being executed on every render with a JavaScript
expression that performs multiplication.
###### Why this matters
This causes unnecessary recalculation of the CSS value on each component
render, potentially impacting rendering performance when the component updates
frequently.
###### Suggested change ā *Feature Preview*
Pre-calculate the value outside the styled component or use CSS custom
properties to avoid runtime calculations:
```typescript
const headerPadding = theme.sizeUnit * 4;
// Then use: max-width: calc(100% - ${headerPadding}px);
```
Or move the calculation to a CSS variable.
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/296672e4-a154-4b1c-ae89-36f644dbe593/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/296672e4-a154-4b1c-ae89-36f644dbe593?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/296672e4-a154-4b1c-ae89-36f644dbe593?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/296672e4-a154-4b1c-ae89-36f644dbe593?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/296672e4-a154-4b1c-ae89-36f644dbe593)
</details>
<sub>
š¬ Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:88d52e59-316e-492d-ba01-57ab9aa44d7d -->
[](88d52e59-316e-492d-ba01-57ab9aa44d7d)
##########
superset-frontend/src/components/Chart/useDrillDetailMenuItems/index.tsx:
##########
@@ -191,79 +172,91 @@ 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: (
- <>
+ onClick: openModal.bind(null, [filter]),
+ label: (
+ <TruncatedMenuLabel
+ tooltipText={`${DRILL_TO_DETAIL_BY} ${filter.formattedVal}`}
+ aria-label={`${DRILL_TO_DETAIL_BY} ${filter.formattedVal}`}
+ >
{`${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>
- </>
+ </TruncatedMenuLabel>
),
- }),
- },
- ].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),
Review Comment:
### Repeated function binding for all filters handler <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
Using Function.prototype.bind() creates a new bound function on every render
for the 'all filters' menu item.
###### Why this matters
This prevents React from optimizing re-renders and creates unnecessary
function allocations on each render cycle.
###### Suggested change ā *Feature Preview*
Use useCallback to memoize the handler:
```typescript
const handleAllFiltersClick = useCallback((event) => {
openModal(filters, event);
}, [openModal, filters]);
// Then use:
onClick: handleAllFiltersClick
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/b565aec1-c4a0-4454-9776-48d598d7cb9d/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/b565aec1-c4a0-4454-9776-48d598d7cb9d?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/b565aec1-c4a0-4454-9776-48d598d7cb9d?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/b565aec1-c4a0-4454-9776-48d598d7cb9d?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/b565aec1-c4a0-4454-9776-48d598d7cb9d)
</details>
<sub>
š¬ Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:f5d3627f-4238-49cd-a437-8711fffea837 -->
[](f5d3627f-4238-49cd-a437-8711fffea837)
##########
superset-frontend/src/components/Chart/DrillBy/DrillBySubmenu.tsx:
##########
@@ -0,0 +1,338 @@
+/**
+ * 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);
+ onSelection(column, drillByConfig);
+ if (openNewModal && onDrillBy && dataset) {
+ onDrillBy(column, dataset);
+ }
+ setPopoverOpen(false);
+ onCloseMenu();
+ },
+ [
+ drillByConfig,
+ onClick,
+ onSelection,
+ openNewModal,
+ onDrillBy,
+ dataset,
+ onCloseMenu,
+ ],
+ );
+
+ useEffect(() => {
+ if (popoverOpen) {
+ // Small delay to ensure popover is rendered
+ setTimeout(() => {
+ ref.current?.input?.focus({ preventScroll: true });
+ }, 100);
Review Comment:
### Uncleaned setTimeout in useEffect <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The setTimeout is not cleaned up if the component unmounts or popoverOpen
changes before the timeout executes.
###### Why this matters
This can cause the focus operation to execute on an unmounted component,
potentially leading to memory leaks and React warnings about setting state on
unmounted components.
###### Suggested change ā *Feature Preview*
Store the timeout ID and clear it in the cleanup function:
```typescript
useEffect(() => {
if (popoverOpen) {
const timeoutId = setTimeout(() => {
ref.current?.input?.focus({ preventScroll: true });
}, 100);
return () => clearTimeout(timeoutId);
} else {
setSearchInput('');
setDebouncedSearchInput('');
}
}, [popoverOpen]);
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/5b049cf3-2a4d-4bbe-9fe1-fe387ebff421/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/5b049cf3-2a4d-4bbe-9fe1-fe387ebff421?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/5b049cf3-2a4d-4bbe-9fe1-fe387ebff421?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/5b049cf3-2a4d-4bbe-9fe1-fe387ebff421?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/5b049cf3-2a4d-4bbe-9fe1-fe387ebff421)
</details>
<sub>
š¬ Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:b0bd11c6-c6a9-4903-b059-ccab71f927d0 -->
[](b0bd11c6-c6a9-4903-b059-ccab71f927d0)
##########
superset-frontend/src/components/Chart/DrillBy/DrillBySubmenu.tsx:
##########
@@ -0,0 +1,338 @@
+/**
+ * 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;
+}
Review Comment:
### Loose Type Definitions <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The interface uses loose typing with 'any' in onSelection callback and
extends formData with an index signature allowing any additional properties.
###### Why this matters
Loose typing reduces type safety and makes the component's API less
predictable, potentially leading to runtime errors that could have been caught
during compilation.
###### Suggested change ā *Feature Preview*
Define strict types for the callbacks and formData:
```typescript
export interface DrillBySubmenuProps {
drillByConfig?: ContextMenuFilters['drillBy'];
formData: BaseFormData & {
viz_type: string;
matrixify_enable_vertical_layout?: boolean;
matrixify_enable_horizontal_layout?: boolean;
};
onSelection?: (column: Column, config: ContextMenuFilters['drillBy']) =>
void;
onClick?: (event: MouseEvent) => void;
...
}
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a5bb7c4b-ede1-4c9e-ad46-41d87886b1c3/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a5bb7c4b-ede1-4c9e-ad46-41d87886b1c3?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a5bb7c4b-ede1-4c9e-ad46-41d87886b1c3?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a5bb7c4b-ede1-4c9e-ad46-41d87886b1c3?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a5bb7c4b-ede1-4c9e-ad46-41d87886b1c3)
</details>
<sub>
š¬ Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:e1e0d972-a037-45fd-99f3-d350541d9d6c -->
[](e1e0d972-a037-45fd-99f3-d350541d9d6c)
##########
superset-frontend/src/components/Chart/DrillBy/DrillBySubmenu.tsx:
##########
@@ -0,0 +1,338 @@
+/**
+ * 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);
+ onSelection(column, drillByConfig);
+ if (openNewModal && onDrillBy && dataset) {
+ onDrillBy(column, dataset);
+ }
+ setPopoverOpen(false);
+ onCloseMenu();
+ },
+ [
+ drillByConfig,
+ onClick,
+ onSelection,
+ openNewModal,
+ onDrillBy,
+ dataset,
+ onCloseMenu,
+ ],
+ );
+
+ useEffect(() => {
+ if (popoverOpen) {
+ // Small delay to ensure popover is rendered
+ setTimeout(() => {
+ ref.current?.input?.focus({ preventScroll: true });
+ }, 100);
Review Comment:
### Missing setTimeout cleanup in useEffect <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The setTimeout is not cleaned up when the component unmounts or when
popoverOpen changes before the timeout executes, potentially causing focus to
be called on an unmounted component.
###### Why this matters
This could lead to memory leaks and React warnings about setting state on
unmounted components, especially if the user quickly opens and closes the
popover.
###### Suggested change ā *Feature Preview*
Store the timeout ID and clear it in the cleanup function:
```typescript
useEffect(() => {
let timeoutId: NodeJS.Timeout;
if (popoverOpen) {
timeoutId = setTimeout(() => {
ref.current?.input?.focus({ preventScroll: true });
}, 100);
} else {
setSearchInput('');
setDebouncedSearchInput('');
}
return () => {
if (timeoutId) {
clearTimeout(timeoutId);
}
};
}, [popoverOpen]);
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/fa49b488-57d2-4bca-b15b-49a2e4bad407/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/fa49b488-57d2-4bca-b15b-49a2e4bad407?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/fa49b488-57d2-4bca-b15b-49a2e4bad407?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/fa49b488-57d2-4bca-b15b-49a2e4bad407?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/fa49b488-57d2-4bca-b15b-49a2e4bad407)
</details>
<sub>
š¬ Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:f7f14a59-e943-4db8-8cf4-2270bab6c412 -->
[](f7f14a59-e943-4db8-8cf4-2270bab6c412)
##########
superset-frontend/src/components/Chart/DrillBy/DrillBySubmenu.tsx:
##########
@@ -0,0 +1,338 @@
+/**
+ * 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);
+ onSelection(column, drillByConfig);
+ if (openNewModal && onDrillBy && dataset) {
+ onDrillBy(column, dataset);
+ }
+ setPopoverOpen(false);
+ onCloseMenu();
+ },
+ [
+ drillByConfig,
+ onClick,
+ onSelection,
+ openNewModal,
+ onDrillBy,
+ dataset,
+ onCloseMenu,
+ ],
+ );
+
+ useEffect(() => {
+ if (popoverOpen) {
+ // Small delay to ensure popover is rendered
+ setTimeout(() => {
+ ref.current?.input?.focus({ preventScroll: true });
+ }, 100);
+ } else {
+ // Reset search input when menu is closed
+ setSearchInput('');
+ setDebouncedSearchInput('');
+ }
+ }, [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),
+ [],
+ );
+
+ const handleInput = (value: string) => {
+ setSearchInput(value);
+ debouncedSetSearchInput(value);
+ };
+
+ const filteredColumns = useMemo(
+ () =>
+ columns
+ .filter(column => {
+ // Filter out excluded columns
+ const excludedColumnNames =
+ excludedColumns?.map(col => col.column_name) || [];
+ return !excludedColumnNames.includes(column.column_name);
+ })
+ .filter(column =>
+ (column.verbose_name || column.column_name)
+ .toLowerCase()
+ .includes(debouncedSearchInput.toLowerCase()),
+ ),
+ [columns, debouncedSearchInput, excludedColumns],
+ );
Review Comment:
### Inefficient excluded columns filtering <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The excludedColumnNames array is recreated on every filter iteration,
causing O(nĀ²) time complexity for the exclusion check.
###### Why this matters
For large datasets, this creates unnecessary computational overhead as the
map operation and array creation happens for each column being filtered.
###### Suggested change ā *Feature Preview*
Pre-compute the excluded column names outside the filter:
```typescript
const filteredColumns = useMemo(() => {
const excludedColumnNames = new Set(
excludedColumns?.map(col => col.column_name) || []
);
return columns
.filter(column => !excludedColumnNames.has(column.column_name))
.filter(column =>
(column.verbose_name || column.column_name)
.toLowerCase()
.includes(debouncedSearchInput.toLowerCase()),
);
}, [columns, debouncedSearchInput, excludedColumns]);
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/835c6c96-2be6-44cc-a303-b26b38b9ed23/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/835c6c96-2be6-44cc-a303-b26b38b9ed23?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/835c6c96-2be6-44cc-a303-b26b38b9ed23?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/835c6c96-2be6-44cc-a303-b26b38b9ed23?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/835c6c96-2be6-44cc-a303-b26b38b9ed23)
</details>
<sub>
š¬ Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:2aa86a34-9031-4883-8031-3ddee6f9c226 -->
[](2aa86a34-9031-4883-8031-3ddee6f9c226)
##########
superset-frontend/src/components/Chart/DrillBy/DrillBySubmenu.tsx:
##########
@@ -0,0 +1,338 @@
+/**
+ * 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);
+ onSelection(column, drillByConfig);
+ if (openNewModal && onDrillBy && dataset) {
+ onDrillBy(column, dataset);
+ }
+ setPopoverOpen(false);
+ onCloseMenu();
+ },
Review Comment:
### Event type mismatch in handleSelection callback <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The handleSelection callback receives an event parameter but the onClick
prop expects a MouseEvent type, while the actual event passed could be a React
SyntheticEvent from the VirtualizedMenuItem click handler.
###### Why this matters
This type mismatch could cause runtime errors or unexpected behavior when
the onClick handler tries to access MouseEvent-specific properties that don't
exist on SyntheticEvent objects.
###### Suggested change ā *Feature Preview*
Update the onClick prop type to accept React's SyntheticEvent or cast the
event parameter:
```typescript
// Option 1: Update the prop type
onClick?: (event: React.MouseEvent | MouseEvent) => void;
// Option 2: Cast the event in handleSelection
const handleSelection = useCallback(
(event, column) => {
onClick(event as MouseEvent);
// ... rest of the function
},
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/87a4d6de-a617-41d3-9563-cdd8cad3f800/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/87a4d6de-a617-41d3-9563-cdd8cad3f800?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/87a4d6de-a617-41d3-9563-cdd8cad3f800?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/87a4d6de-a617-41d3-9563-cdd8cad3f800?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/87a4d6de-a617-41d3-9563-cdd8cad3f800)
</details>
<sub>
š¬ Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:db5331ff-4891-45d4-8c08-7179f009184b -->
[](db5331ff-4891-45d4-8c08-7179f009184b)
##########
superset-frontend/src/components/Chart/ChartContextMenu/ChartContextMenu.tsx:
##########
@@ -264,115 +257,76 @@ const ChartContextMenu = (
itemsCount = 1; // "No actions" appears if no actions in menu
}
+ const drillDetailMenuItems = useDrillDetailMenuItems({
+ formData: drillFormData,
+ filters: filters?.drillToDetail,
+ setFilters,
+ isContextMenu: true,
+ contextMenuY: clientY,
+ onSelection,
+ submenuIndex: showCrossFilters ? 2 : 1,
+ setShowModal: setDrillModalIsOpen,
+ dataset: filteredDataset,
+ isLoadingDataset,
+ ...(additionalConfig?.drillToDetail || {}),
+ });
Review Comment:
### Unconditional hook execution for drill detail menu items <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The useDrillDetailMenuItems hook is called unconditionally on every render,
even when drill-to-detail functionality is not shown or needed.
###### Why this matters
This causes unnecessary computation and potential API calls or expensive
operations within the hook, degrading performance especially when the context
menu is frequently re-rendered or when drill-to-detail is disabled.
###### Suggested change ā *Feature Preview*
Move the hook call inside a conditional block or use useMemo with proper
dependencies to only execute when showDrillToDetail is true:
```typescript
const drillDetailMenuItems = useMemo(() => {
if (!showDrillToDetail) return [];
return useDrillDetailMenuItems({
formData: drillFormData,
filters: filters?.drillToDetail,
setFilters,
isContextMenu: true,
contextMenuY: clientY,
onSelection,
submenuIndex: showCrossFilters ? 2 : 1,
setShowModal: setDrillModalIsOpen,
dataset: filteredDataset,
isLoadingDataset,
...(additionalConfig?.drillToDetail || {}),
});
}, [showDrillToDetail, drillFormData, filters?.drillToDetail, /* other deps
*/]);
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/c800734d-6fe1-4146-8a22-c589c7b298de/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/c800734d-6fe1-4146-8a22-c589c7b298de?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/c800734d-6fe1-4146-8a22-c589c7b298de?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/c800734d-6fe1-4146-8a22-c589c7b298de?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/c800734d-6fe1-4146-8a22-c589c7b298de)
</details>
<sub>
š¬ Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:9e28002a-47d4-495c-8a8c-18e452e6b137 -->
[](9e28002a-47d4-495c-8a8c-18e452e6b137)
##########
superset-frontend/src/components/Chart/useDrillDetailMenuItems/index.tsx:
##########
@@ -191,79 +172,91 @@ 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,
};
Review Comment:
### Duplicated menu item creation logic <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
Menu item creation logic is duplicated and mixed with presentation concerns,
making it difficult to maintain consistent styling and behavior across menu
items.
###### Why this matters
Violates DRY principle and mixes concerns, making it harder to maintain
consistent menu item styling and behavior. Changes to menu item structure
require updates in multiple places.
###### Suggested change ā *Feature Preview*
Extract menu item creation into a reusable factory function:
```typescript
const createMenuItem = ({
key,
label,
disabled,
onClick,
tooltipTitle,
...rest
}: MenuItemConfig): ItemType => ({
key,
label: disabled ? (
<DisabledMenuItemLabel label={label} tooltipTitle={tooltipTitle} />
) : label,
disabled,
onClick,
...rest
});
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/98cbf2aa-34bf-466c-8e98-d0eb51073fdb/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/98cbf2aa-34bf-466c-8e98-d0eb51073fdb?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/98cbf2aa-34bf-466c-8e98-d0eb51073fdb?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/98cbf2aa-34bf-466c-8e98-d0eb51073fdb?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/98cbf2aa-34bf-466c-8e98-d0eb51073fdb)
</details>
<sub>
š¬ Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:1dbac2a1-e4ce-43ea-8a25-5d789f0265e6 -->
[](1dbac2a1-e4ce-43ea-8a25-5d789f0265e6)
--
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]
