korbit-ai[bot] commented on code in PR #33104: URL: https://github.com/apache/superset/pull/33104#discussion_r2040286902
########## superset-frontend/src/explore/components/DatasourcePanel/DatasourceItems.tsx: ########## @@ -0,0 +1,151 @@ +/** + * 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 { useCallback, useEffect, useMemo, useState } from 'react'; +import { VariableSizeList as List } from 'react-window'; +import { cloneDeep } from 'lodash'; +import { FlattenedItem, Folder } from './types'; +import DatasourcePanelItem from './DatasourcePanelItem'; + +const BORDER_WIDTH = 2; +const HEADER_ITEM_HEIGHT = 50; +const METRIC_OR_COLUMN_ITEM_HEIGHT = 32; +const DIVIDER_ITEM_HEIGHT = 16; + +const flattenFolderStructure = ( + folders: Folder[], + depth = 0, + folderMap: Map<string, Folder> = new Map(), +): { flattenedItems: FlattenedItem[]; folderMap: Map<string, Folder> } => { + const flattenedItems: FlattenedItem[] = []; + + folders.forEach((folder, idx) => { + folderMap.set(folder.id, folder); + + flattenedItems.push({ + type: 'header', + folderId: folder.id, + depth, + height: HEADER_ITEM_HEIGHT, + }); + + if (!folder.isCollapsed) { + folder.items.forEach(item => { + flattenedItems.push({ + type: 'item', + folderId: folder.id, + depth, + item, + height: METRIC_OR_COLUMN_ITEM_HEIGHT, + }); + }); + + if (folder.subFolders && folder.subFolders.length > 0) { + const { flattenedItems: subItems } = flattenFolderStructure( + folder.subFolders, + depth + 1, + folderMap, + ); + + flattenedItems.push(...subItems); + } + } + if (depth === 0 && idx !== folders.length - 1) { + flattenedItems.push({ + type: 'divider', + folderId: folder.id, + depth, + height: DIVIDER_ITEM_HEIGHT, + }); + } + }); + + return { flattenedItems, folderMap }; +}; + +interface DatasourceItemsProps { + width: number; + height: number; + folders: Folder[]; +} +export const DatasourceItems = ({ + width, + height, + folders, +}: DatasourceItemsProps) => { + const [folderStructure, setFolderStructure] = useState<Folder[]>(folders); + + useEffect(() => { + setFolderStructure(prev => (prev !== folders ? folders : prev)); + }, [folders]); + + const { flattenedItems, folderMap } = useMemo( + () => flattenFolderStructure(folderStructure), + [folderStructure], + ); + + const handleToggleCollapse = useCallback((folderId: string) => { + setFolderStructure(prevFolders => { + const updatedFolders = cloneDeep(prevFolders); Review Comment: ### Inefficient Deep Clone in Toggle Operation <sub></sub> <details> <summary>Tell me more</summary> ###### What is the issue? Using lodash's cloneDeep for the entire folder structure on every collapse toggle operation is unnecessarily expensive. ###### Why this matters Deep cloning the entire folder structure creates a significant performance overhead, especially with large datasets, as it copies all nested objects and arrays regardless of whether they're affected by the toggle operation. ###### Suggested change ∙ *Feature Preview* Implement a selective update that only clones the minimal path to the modified folder. Example approach: ```typescript const updatedFolders = prevFolders.map(folder => updateFolderPath(folder, folderId) ); function updateFolderPath(folder: Folder, targetId: string): Folder { if (folder.id === targetId) { return { ...folder, isCollapsed: !folder.isCollapsed }; } if (folder.subFolders) { return { ...folder, subFolders: folder.subFolders.map(sub => updateFolderPath(sub, targetId)) }; } return folder; } ``` ###### Provide feedback to improve future suggestions [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/c4c3015c-f04f-42f8-aad8-31104cf11c6a/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/c4c3015c-f04f-42f8-aad8-31104cf11c6a?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/c4c3015c-f04f-42f8-aad8-31104cf11c6a?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/c4c3015c-f04f-42f8-aad8-31104cf11c6a?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/c4c3015c-f04f-42f8-aad8-31104cf11c6a) </details> <sub> 💬 Looking for more details? Reply to this comment to chat with Korbit. </sub> <!--- korbi internal id:9a887d51-ebbf-475e-8644-ac02d765b75a --> [](9a887d51-ebbf-475e-8644-ac02d765b75a) ########## superset-frontend/src/explore/components/DatasourcePanel/DatasourcePanelItem.tsx: ########## @@ -123,146 +76,95 @@ border: none; background: transparent; width: 100%; - padding-inline: 0px; + height: 100%; + padding-inline: 0; `; const SectionHeader = styled.span` - ${({ theme }) => ` + ${({ theme }) => css` + color: ${theme.colors.grayscale.dark1}; font-size: ${theme.typography.sizes.m}px; + font-weight: ${theme.typography.weights.medium}; line-height: 1.3; `} `; -const Box = styled.div` - ${({ theme }) => ` - border: 1px ${theme.colors.grayscale.light4} solid; - border-radius: ${theme.gridUnit}px; - font-size: ${theme.typography.sizes.s}px; - padding: ${theme.gridUnit}px; - color: ${theme.colors.grayscale.light1}; - text-overflow: ellipsis; - white-space: nowrap; - overflow: hidden; +const Divider = styled.div` + ${({ theme }) => css` + height: 16px; + border-bottom: 1px solid ${theme.colors.grayscale.light3}; `} `; -const DatasourcePanelItem: FC<Props> = ({ index, style, data }) => { - const { - metricSlice: _metricSlice, - columnSlice, - totalMetrics, - totalColumns, - width, - showAllMetrics, - onShowAllMetricsChange, - showAllColumns, - onShowAllColumnsChange, - collapseMetrics, - onCollapseMetricsChange, - collapseColumns, - onCollapseColumnsChange, - hiddenMetricCount, - hiddenColumnCount, - } = data; - const metricSlice = collapseMetrics ? [] : _metricSlice; - - const EXTRA_LINES = collapseMetrics ? 1 : 2; - const isColumnSection = collapseMetrics - ? index >= 1 - : index > metricSlice.length + EXTRA_LINES; - const HEADER_LINE = isColumnSection - ? metricSlice.length + EXTRA_LINES + 1 - : 0; - const SUBTITLE_LINE = HEADER_LINE + 1; - const BOTTOM_LINE = - (isColumnSection ? columnSlice.length : metricSlice.length) + - (collapseMetrics ? HEADER_LINE : SUBTITLE_LINE) + - 1; - const collapsed = isColumnSection ? collapseColumns : collapseMetrics; - const setCollapse = isColumnSection - ? onCollapseColumnsChange - : onCollapseMetricsChange; - const showAll = isColumnSection ? showAllColumns : showAllMetrics; - const setShowAll = isColumnSection - ? onShowAllColumnsChange - : onShowAllMetricsChange; +export interface DatasourcePanelItemProps { + index: number; + style: CSSProperties; + data: { + flattenedItems: FlattenedItem[]; + folderMap: Map<string, Folder>; + width: number; + onToggleCollapse: (folderId: string) => void; + }; +} + +const DatasourcePanelItem = ({ + index, + style, + data, +}: DatasourcePanelItemProps) => { + const { flattenedItems, folderMap, width, onToggleCollapse } = data; + const item = flattenedItems[index]; const theme = useTheme(); - const hiddenCount = isColumnSection ? hiddenColumnCount : hiddenMetricCount; + + if (!item) return null; + + const folder = folderMap.get(item.folderId); + if (!folder) return null; + + const indentation = item.depth * theme.gridUnit * 4; return ( <div - style={style} - css={css` - padding: 0 ${theme.gridUnit * 4}px; - `} + style={{ + ...style, + paddingLeft: theme.gridUnit * 4 + indentation, + paddingRight: theme.gridUnit * 4, + }} > - {index === HEADER_LINE && ( - <SectionHeaderButton onClick={() => setCollapse(!collapsed)}> - <SectionHeader> - {isColumnSection ? t('Columns') : t('Metrics')} - </SectionHeader> - {collapsed ? ( + {item.type === 'header' && ( + <SectionHeaderButton onClick={() => onToggleCollapse(folder.id)}> + <SectionHeader>{folder.name}</SectionHeader> + {folder.isCollapsed ? ( <Icons.DownOutlined iconSize="s" /> ) : ( <Icons.UpOutlined iconSize="s" /> )} </SectionHeaderButton> )} - {index === SUBTITLE_LINE && !collapsed && ( - <div - css={css` - display: flex; - gap: ${theme.gridUnit * 2}px; - justify-content: space-between; - align-items: baseline; - `} - > - <div - className="field-length" - css={css` - flex-shrink: 0; - `} - > - {isColumnSection - ? t(`Showing %s of %s`, columnSlice?.length, totalColumns) - : t(`Showing %s of %s`, metricSlice?.length, totalMetrics)} - </div> - {hiddenCount > 0 && ( - <Box>{t(`%s ineligible item(s) are hidden`, hiddenCount)}</Box> - )} - </div> - )} - {index > SUBTITLE_LINE && index < BOTTOM_LINE && ( + + {item.type === 'item' && item.item && ( <LabelWrapper key={ - (isColumnSection - ? columnSlice[index - SUBTITLE_LINE - 1].column_name - : metricSlice[index - SUBTITLE_LINE - 1].metric_name) + - String(width) + (item.item.type === 'column' + ? item.item.column_name + : item.item.metric_name) + String(width) } className="column" Review Comment: ### Misleading Class Name <sub></sub> <details> <summary>Tell me more</summary> ###### What is the issue? The className 'column' is misleading as the LabelWrapper can contain both columns and metrics. ###### Why this matters Misleading class names can confuse developers during maintenance and debugging sessions. ###### Suggested change ∙ *Feature Preview* ```typescript className="datasource-panel-item" ``` ###### Provide feedback to improve future suggestions [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/3e25130a-da29-49fa-a527-dabae6d0bd27/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/3e25130a-da29-49fa-a527-dabae6d0bd27?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/3e25130a-da29-49fa-a527-dabae6d0bd27?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/3e25130a-da29-49fa-a527-dabae6d0bd27?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/3e25130a-da29-49fa-a527-dabae6d0bd27) </details> <sub> 💬 Looking for more details? Reply to this comment to chat with Korbit. </sub> <!--- korbi internal id:34572b5b-467c-4ff0-9873-127c78af3cf7 --> [](34572b5b-467c-4ff0-9873-127c78af3cf7) ########## superset-frontend/src/explore/components/DatasourcePanel/index.tsx: ########## @@ -126,8 +122,18 @@ const StyledInfoboxWrapper = styled.div` const BORDER_WIDTH = 2; -const sortCertifiedFirst = (slice: DataSourcePanelColumn[]) => - slice.sort((a, b) => (b?.is_certified ?? 0) - (a?.is_certified ?? 0)); +const sortColumns = (slice: DatasourcePanelColumn[]) => + [...slice] + .sort((col1, col2) => { + if (col1?.is_dttm && !col2?.is_dttm) { + return -1; + } + if (col2?.is_dttm && !col1?.is_dttm) { + return 1; + } + return 0; + }) + .sort((a, b) => (b?.is_certified ?? 0) - (a?.is_certified ?? 0)); Review Comment: ### Unclear Multi-criteria Sort Implementation <sub></sub> <details> <summary>Tell me more</summary> ###### What is the issue? The sorting function combines two different sorting criteria (is_dttm and is_certified) in a way that's not immediately clear and uses chained .sort() calls which can be confusing. ###### Why this matters Chained sorts can be harder to understand and maintain. The current implementation makes it difficult to quickly grasp the complete sorting priority logic. ###### Suggested change ∙ *Feature Preview* ```typescript const sortColumns = (slice: DatasourcePanelColumn[]) => { return [...slice].sort((col1, col2) => { // First priority: temporal columns (is_dttm) const dttmDiff = Number(col2?.is_dttm) - Number(col1?.is_dttm); if (dttmDiff !== 0) return dttmDiff; // Second priority: certified columns return (col2?.is_certified ?? 0) - (col1?.is_certified ?? 0); }); }; ``` ###### Provide feedback to improve future suggestions [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a3c70d30-ef8a-4265-a721-5a8e784b5e43/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a3c70d30-ef8a-4265-a721-5a8e784b5e43?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a3c70d30-ef8a-4265-a721-5a8e784b5e43?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a3c70d30-ef8a-4265-a721-5a8e784b5e43?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a3c70d30-ef8a-4265-a721-5a8e784b5e43) </details> <sub> 💬 Looking for more details? Reply to this comment to chat with Korbit. </sub> <!--- korbi internal id:ba105622-95b0-46cc-98ac-c4e7453dc23d --> [](ba105622-95b0-46cc-98ac-c4e7453dc23d) ########## superset-frontend/src/explore/components/DatasourcePanel/transformDatasourceFolders.ts: ########## @@ -0,0 +1,155 @@ +/** + * 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 { Metric, t } from '@superset-ui/core'; +import { + ColumnItem, + DatasourceFolder, + DatasourcePanelColumn, + Folder, + MetricItem, +} from './types'; + +const transformToFolderStructure = ( + metrics: MetricItem[], + columns: ColumnItem[], + folderConfig: DatasourceFolder[] | undefined, +): Folder[] => { + const metricsMap = new Map<string, MetricItem>(); + const columnsMap = new Map<string, ColumnItem>(); + + const assignedMetricUuids = new Set<string>(); + const assignedColumnUuids = new Set<string>(); + + metrics.forEach(metric => { + metricsMap.set(metric.uuid, metric); + }); + + columns.forEach(column => { + columnsMap.set(column.uuid, column); + }); + + const processFolder = ( + datasourceFolder: DatasourceFolder, + parentId?: string, + ): Folder => { + const folder: Folder = { + id: datasourceFolder.uuid, + name: datasourceFolder.name, + description: datasourceFolder.description, + isCollapsed: false, + items: [], + parentId, + }; + + if (datasourceFolder.children && datasourceFolder.children.length > 0) { + if (!folder.subFolders) { + folder.subFolders = []; + } Review Comment: ### Premature Subfolder Array Creation <sub></sub> <details> <summary>Tell me more</summary> ###### What is the issue? The code unnecessarily creates an empty subFolders array when children exist, even if none of those children are folders, which could lead to confusing folder structures with empty subfolder arrays. ###### Why this matters This can cause unnecessary memory allocation and potentially misleading folder structure representation when all children are metrics or columns. ###### Suggested change ∙ *Feature Preview* Move the subFolders array creation into the folder type check: ```typescript if (datasourceFolder.children && datasourceFolder.children.length > 0) { datasourceFolder.children.forEach(child => { if (child.type === 'folder') { if (!folder.subFolders) { folder.subFolders = []; } folder.subFolders.push( processFolder(child as DatasourceFolder, folder.id), ); } else if (child.type === 'metric') { // ... rest of the code ``` ###### Provide feedback to improve future suggestions [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/af2f73c2-240a-413c-b9b1-3e5a62b21077/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/af2f73c2-240a-413c-b9b1-3e5a62b21077?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/af2f73c2-240a-413c-b9b1-3e5a62b21077?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/af2f73c2-240a-413c-b9b1-3e5a62b21077?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/af2f73c2-240a-413c-b9b1-3e5a62b21077) </details> <sub> 💬 Looking for more details? Reply to this comment to chat with Korbit. </sub> <!--- korbi internal id:9c653916-4794-49ce-bbb6-5363398776cd --> [](9c653916-4794-49ce-bbb6-5363398776cd) ########## superset-frontend/src/explore/components/DatasourcePanel/DatasourcePanelItem.tsx: ########## @@ -123,146 +76,95 @@ const SectionHeaderButton = styled.button` border: none; background: transparent; width: 100%; - padding-inline: 0px; + height: 100%; + padding-inline: 0; `; const SectionHeader = styled.span` - ${({ theme }) => ` + ${({ theme }) => css` + color: ${theme.colors.grayscale.dark1}; font-size: ${theme.typography.sizes.m}px; + font-weight: ${theme.typography.weights.medium}; line-height: 1.3; `} `; -const Box = styled.div` - ${({ theme }) => ` - border: 1px ${theme.colors.grayscale.light4} solid; - border-radius: ${theme.gridUnit}px; - font-size: ${theme.typography.sizes.s}px; - padding: ${theme.gridUnit}px; - color: ${theme.colors.grayscale.light1}; - text-overflow: ellipsis; - white-space: nowrap; - overflow: hidden; +const Divider = styled.div` + ${({ theme }) => css` + height: 16px; + border-bottom: 1px solid ${theme.colors.grayscale.light3}; `} `; -const DatasourcePanelItem: FC<Props> = ({ index, style, data }) => { - const { - metricSlice: _metricSlice, - columnSlice, - totalMetrics, - totalColumns, - width, - showAllMetrics, - onShowAllMetricsChange, - showAllColumns, - onShowAllColumnsChange, - collapseMetrics, - onCollapseMetricsChange, - collapseColumns, - onCollapseColumnsChange, - hiddenMetricCount, - hiddenColumnCount, - } = data; - const metricSlice = collapseMetrics ? [] : _metricSlice; - - const EXTRA_LINES = collapseMetrics ? 1 : 2; - const isColumnSection = collapseMetrics - ? index >= 1 - : index > metricSlice.length + EXTRA_LINES; - const HEADER_LINE = isColumnSection - ? metricSlice.length + EXTRA_LINES + 1 - : 0; - const SUBTITLE_LINE = HEADER_LINE + 1; - const BOTTOM_LINE = - (isColumnSection ? columnSlice.length : metricSlice.length) + - (collapseMetrics ? HEADER_LINE : SUBTITLE_LINE) + - 1; - const collapsed = isColumnSection ? collapseColumns : collapseMetrics; - const setCollapse = isColumnSection - ? onCollapseColumnsChange - : onCollapseMetricsChange; - const showAll = isColumnSection ? showAllColumns : showAllMetrics; - const setShowAll = isColumnSection - ? onShowAllColumnsChange - : onShowAllMetricsChange; +export interface DatasourcePanelItemProps { + index: number; + style: CSSProperties; + data: { + flattenedItems: FlattenedItem[]; + folderMap: Map<string, Folder>; + width: number; + onToggleCollapse: (folderId: string) => void; + }; +} + +const DatasourcePanelItem = ({ + index, + style, + data, +}: DatasourcePanelItemProps) => { + const { flattenedItems, folderMap, width, onToggleCollapse } = data; + const item = flattenedItems[index]; const theme = useTheme(); - const hiddenCount = isColumnSection ? hiddenColumnCount : hiddenMetricCount; + + if (!item) return null; + + const folder = folderMap.get(item.folderId); + if (!folder) return null; + + const indentation = item.depth * theme.gridUnit * 4; Review Comment: ### Unexplained Magic Number <sub></sub> <details> <summary>Tell me more</summary> ###### What is the issue? Magic number 4 is used in indentation calculation without explanation of its significance. ###### Why this matters Unexplained magic numbers make it harder to understand the intent and make future modifications more error-prone. ###### Suggested change ∙ *Feature Preview* ```typescript const INDENTATION_MULTIPLIER = 4; // Number of grid units per depth level const indentation = item.depth * theme.gridUnit * INDENTATION_MULTIPLIER; ``` ###### Provide feedback to improve future suggestions [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/f585c07b-0d44-4d1e-a014-a00e1e024f41/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/f585c07b-0d44-4d1e-a014-a00e1e024f41?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/f585c07b-0d44-4d1e-a014-a00e1e024f41?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/f585c07b-0d44-4d1e-a014-a00e1e024f41?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/f585c07b-0d44-4d1e-a014-a00e1e024f41) </details> <sub> 💬 Looking for more details? Reply to this comment to chat with Korbit. </sub> <!--- korbi internal id:2ebde5e3-eaff-4358-8e80-f26dee8292dc --> [](2ebde5e3-eaff-4358-8e80-f26dee8292dc) ########## superset-frontend/src/explore/components/DatasourcePanel/transformDatasourceFolders.ts: ########## @@ -0,0 +1,155 @@ +/** + * 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 { Metric, t } from '@superset-ui/core'; +import { + ColumnItem, + DatasourceFolder, + DatasourcePanelColumn, + Folder, + MetricItem, +} from './types'; + +const transformToFolderStructure = ( + metrics: MetricItem[], + columns: ColumnItem[], + folderConfig: DatasourceFolder[] | undefined, +): Folder[] => { Review Comment: ### Ambiguous Function Name <sub></sub> <details> <summary>Tell me more</summary> ###### What is the issue? The function name 'transformToFolderStructure' does not clearly convey that it also handles the case of missing folder configuration by creating default folders. ###### Why this matters Without clear indication of the default behavior in the function name, developers might be surprised by the creation of default folders when folderConfig is undefined. ###### Suggested change ∙ *Feature Preview* Rename function to `transformToFolderStructureWithDefaults` or split into two separate functions: `createDefaultFolderStructure` and `transformToFolderStructure` ###### Provide feedback to improve future suggestions [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a25d2c3b-4e0a-4c9d-b596-1c1675cf0ef8/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a25d2c3b-4e0a-4c9d-b596-1c1675cf0ef8?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a25d2c3b-4e0a-4c9d-b596-1c1675cf0ef8?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a25d2c3b-4e0a-4c9d-b596-1c1675cf0ef8?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a25d2c3b-4e0a-4c9d-b596-1c1675cf0ef8) </details> <sub> 💬 Looking for more details? Reply to this comment to chat with Korbit. </sub> <!--- korbi internal id:cd628276-7a08-4ab6-900b-28cca068f1c3 --> [](cd628276-7a08-4ab6-900b-28cca068f1c3) ########## superset-frontend/src/explore/components/DatasourcePanel/DatasourceItems.tsx: ########## @@ -0,0 +1,151 @@ +/** + * 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 { useCallback, useEffect, useMemo, useState } from 'react'; +import { VariableSizeList as List } from 'react-window'; +import { cloneDeep } from 'lodash'; +import { FlattenedItem, Folder } from './types'; +import DatasourcePanelItem from './DatasourcePanelItem'; + +const BORDER_WIDTH = 2; +const HEADER_ITEM_HEIGHT = 50; +const METRIC_OR_COLUMN_ITEM_HEIGHT = 32; +const DIVIDER_ITEM_HEIGHT = 16; + +const flattenFolderStructure = ( + folders: Folder[], + depth = 0, + folderMap: Map<string, Folder> = new Map(), +): { flattenedItems: FlattenedItem[]; folderMap: Map<string, Folder> } => { Review Comment: ### Multiple Responsibilities in Utility Function <sub></sub> <details> <summary>Tell me more</summary> ###### What is the issue? The flattenFolderStructure function handles both flattening the structure and managing a folder map, violating the Single Responsibility Principle. ###### Why this matters Combining these responsibilities makes the function less reusable and harder to test. Changes to either the flattening logic or mapping logic require modifying the same function. ###### Suggested change ∙ *Feature Preview* Split the function into two separate functions: ```typescript const createFolderMap = (folders: Folder[]): Map<string, Folder> => { const map = new Map(); const addToMap = (folder: Folder) => { map.set(folder.id, folder); folder.subFolders?.forEach(addToMap); }; folders.forEach(addToMap); return map; }; const flattenFolderStructure = ( folders: Folder[], depth = 0 ): FlattenedItem[] => { // handle only flattening logic }; ``` ###### Provide feedback to improve future suggestions [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/0a167786-911b-4c0e-8f75-1966e7924268/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/0a167786-911b-4c0e-8f75-1966e7924268?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/0a167786-911b-4c0e-8f75-1966e7924268?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/0a167786-911b-4c0e-8f75-1966e7924268?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/0a167786-911b-4c0e-8f75-1966e7924268) </details> <sub> 💬 Looking for more details? Reply to this comment to chat with Korbit. </sub> <!--- korbi internal id:bd752c3f-53bd-4689-9121-6df192164a74 --> [](bd752c3f-53bd-4689-9121-6df192164a74) ########## superset-frontend/src/explore/components/DatasourcePanel/DatasourceItems.tsx: ########## @@ -0,0 +1,151 @@ +/** + * 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 { useCallback, useEffect, useMemo, useState } from 'react'; +import { VariableSizeList as List } from 'react-window'; +import { cloneDeep } from 'lodash'; +import { FlattenedItem, Folder } from './types'; +import DatasourcePanelItem from './DatasourcePanelItem'; + +const BORDER_WIDTH = 2; +const HEADER_ITEM_HEIGHT = 50; +const METRIC_OR_COLUMN_ITEM_HEIGHT = 32; +const DIVIDER_ITEM_HEIGHT = 16; + +const flattenFolderStructure = ( + folders: Folder[], + depth = 0, + folderMap: Map<string, Folder> = new Map(), +): { flattenedItems: FlattenedItem[]; folderMap: Map<string, Folder> } => { + const flattenedItems: FlattenedItem[] = []; + + folders.forEach((folder, idx) => { + folderMap.set(folder.id, folder); + + flattenedItems.push({ + type: 'header', + folderId: folder.id, + depth, + height: HEADER_ITEM_HEIGHT, + }); + + if (!folder.isCollapsed) { + folder.items.forEach(item => { + flattenedItems.push({ + type: 'item', + folderId: folder.id, + depth, + item, + height: METRIC_OR_COLUMN_ITEM_HEIGHT, + }); + }); + + if (folder.subFolders && folder.subFolders.length > 0) { + const { flattenedItems: subItems } = flattenFolderStructure( + folder.subFolders, + depth + 1, + folderMap, + ); + + flattenedItems.push(...subItems); + } + } + if (depth === 0 && idx !== folders.length - 1) { + flattenedItems.push({ + type: 'divider', + folderId: folder.id, + depth, + height: DIVIDER_ITEM_HEIGHT, + }); + } + }); + + return { flattenedItems, folderMap }; +}; + +interface DatasourceItemsProps { + width: number; + height: number; + folders: Folder[]; +} +export const DatasourceItems = ({ + width, + height, + folders, +}: DatasourceItemsProps) => { + const [folderStructure, setFolderStructure] = useState<Folder[]>(folders); + + useEffect(() => { + setFolderStructure(prev => (prev !== folders ? folders : prev)); + }, [folders]); + + const { flattenedItems, folderMap } = useMemo( + () => flattenFolderStructure(folderStructure), + [folderStructure], + ); + + const handleToggleCollapse = useCallback((folderId: string) => { + setFolderStructure(prevFolders => { + const updatedFolders = cloneDeep(prevFolders); + + const updateFolder = (folders: Folder[] | undefined): boolean => { Review Comment: ### Misleading function name <sub></sub> <details> <summary>Tell me more</summary> ###### What is the issue? The function name 'updateFolder' is misleading as it actually toggles a folder's collapse state rather than performing a general update. ###### Why this matters Imprecise function naming can lead to confusion about the function's purpose and make the code harder to maintain. ###### Suggested change ∙ *Feature Preview* Rename the function to better reflect its specific purpose: ```typescript const toggleFolderCollapse = (folders: Folder[] | undefined): boolean => { ``` ###### Provide feedback to improve future suggestions [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/f860360a-2654-415f-8657-2eabf366271e/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/f860360a-2654-415f-8657-2eabf366271e?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/f860360a-2654-415f-8657-2eabf366271e?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/f860360a-2654-415f-8657-2eabf366271e?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/f860360a-2654-415f-8657-2eabf366271e) </details> <sub> 💬 Looking for more details? Reply to this comment to chat with Korbit. </sub> <!--- korbi internal id:fd3ddc42-459d-45a2-bb57-4c13e4572626 --> [](fd3ddc42-459d-45a2-bb57-4c13e4572626) ########## superset-frontend/src/explore/components/DatasourcePanel/types.ts: ########## @@ -35,3 +35,55 @@ export function isDatasourcePanelDndItem( export function isSavedMetric(item: any): item is Metric { return item?.metric_name; } + +export type DatasourcePanelColumn = { + uuid: string; + id?: number; + is_dttm?: boolean | null; + description?: string | null; + expression?: string | null; + is_certified?: number | null; + column_name?: string | null; + name?: string | null; + type?: string; +}; + +export type DatasourceFolder = { + uuid: string; + type: 'folder'; + name: string; + description?: string; + children?: ( + | DatasourceFolder + | { type: 'metric'; uuid: string; name: string } + | { type: 'column'; uuid: string; name: string } + )[]; +}; Review Comment: ### Redundant Inline Type Definitions <sub></sub> <details> <summary>Tell me more</summary> ###### What is the issue? The children field uses inline type definitions that duplicate properties (uuid, name) already defined in MetricItem and ColumnItem types. ###### Why this matters Duplicating type definitions increases maintenance burden and risk of inconsistency when types need to be updated. ###### Suggested change ∙ *Feature Preview* ```typescript export type DatasourceFolder = { uuid: string; type: 'folder'; name: string; description?: string; children?: (DatasourceFolder | Pick<MetricItem, 'type' | 'uuid' | 'name'> | Pick<ColumnItem, 'type' | 'uuid' | 'name'>)[]; }; ``` ###### Provide feedback to improve future suggestions [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/540e7b03-28c4-4f0c-87ec-5ebbb09b2772/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/540e7b03-28c4-4f0c-87ec-5ebbb09b2772?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/540e7b03-28c4-4f0c-87ec-5ebbb09b2772?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/540e7b03-28c4-4f0c-87ec-5ebbb09b2772?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/540e7b03-28c4-4f0c-87ec-5ebbb09b2772) </details> <sub> 💬 Looking for more details? Reply to this comment to chat with Korbit. </sub> <!--- korbi internal id:f8cdc7fe-cfee-4226-bf24-fd777bddcaa9 --> [](f8cdc7fe-cfee-4226-bf24-fd777bddcaa9) -- 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: notifications-unsubscr...@superset.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org For additional commands, e-mail: notifications-h...@superset.apache.org
