codeant-ai-for-open-source[bot] commented on code in PR #37298: URL: https://github.com/apache/superset/pull/37298#discussion_r2739124912
########## superset-frontend/src/SqlLab/components/TableExploreTree/TreeNodeRenderer.tsx: ########## @@ -0,0 +1,243 @@ +/** + * 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 { css, styled, t } from '@apache-superset/core'; +import type { NodeRendererProps } from 'react-arborist'; +import { Icons, Tooltip, Typography } from '@superset-ui/core/components'; +import RefreshLabel from '@superset-ui/core/components/RefreshLabel'; +import ColumnElement from 'src/SqlLab/components/ColumnElement'; +import IconButton from 'src/dashboard/components/IconButton'; +import type { TreeNodeData, FetchLazyTablesParams } from './types'; + +const StyledColumnNode = styled.div` + & > .ant-flex { + flex: 1; + margin-right: ${({ theme }) => theme.sizeUnit * 1.5}px; + cursor: default; + } +`; + +const getOpacity = (disableCheckbox: boolean | undefined) => + disableCheckbox ? 0.6 : 1; + +const highlightText = (text: string, keyword: string): React.ReactNode => { + if (!keyword) { + return text; + } + + const lowerText = text.toLowerCase(); + const lowerKeyword = keyword.toLowerCase(); + const index = lowerText.indexOf(lowerKeyword); + + if (index === -1) { + return text; + } + + const beforeStr = text.substring(0, index); + const matchStr = text.substring(index, index + keyword.length); + const afterStr = text.slice(index + keyword.length); + + return ( + <> + {beforeStr} + <span className="highlighted">{matchStr}</span> + {afterStr} + </> + ); +}; + +export interface TreeNodeRendererProps extends NodeRendererProps<TreeNodeData> { + manuallyOpenedNodes: Record<string, boolean>; + loadingNodes: Record<string, boolean>; + searchTerm: string; + catalog: string | null | undefined; + fetchLazyTables: (params: FetchLazyTablesParams) => void; + handlePinTable: ( + tableName: string, + schemaName: string, + catalogName: string | null, + ) => void; +} + +const TreeNodeRenderer: React.FC<TreeNodeRendererProps> = ({ + node, + style, + manuallyOpenedNodes, + loadingNodes, + searchTerm, + catalog, + fetchLazyTables, + handlePinTable, +}) => { + const { data } = node; + const parts = data.id.split(':'); + const [identifier, _dbId, schema, tableName] = parts; + + // Use manually tracked open state for icon display + // This prevents search auto-expansion from affecting the icon + const isManuallyOpen = manuallyOpenedNodes[data.id] ?? false; + const isLoading = loadingNodes[data.id] ?? false; + + const renderIcon = () => { + if (identifier === 'schema') { + // Show loading icon when fetching data for schema + if (isLoading) { + return <Icons.LoadingOutlined iconSize="l" />; + } + return isManuallyOpen ? ( + <Icons.FolderOpenOutlined iconSize="l" /> + ) : ( + <Icons.FolderOutlined iconSize="l" /> + ); + } + + if (identifier === 'table') { + const TableTypeIcon = + data.tableType === 'view' ? Icons.EyeOutlined : Icons.TableOutlined; + // Show loading icon with table type icon when loading + if (isLoading) { + return ( + <> + <Icons.LoadingOutlined iconSize="l" /> + <TableTypeIcon iconSize="l" /> + </> + ); + } + const ExpandIcon = isManuallyOpen + ? Icons.MinusSquareOutlined + : Icons.PlusSquareOutlined; + return ( + <> + <ExpandIcon iconSize="l" /> + <TableTypeIcon iconSize="l" /> + </> + ); + } + + return null; + }; + + // Empty placeholder node - no actions allowed + if (data.type === 'empty') { + return ( + <div + className="tree-node" + style={{ + ...style, + opacity: 0.5, + fontStyle: 'italic', + cursor: 'default', + }} + > + <span className="tree-node-icon"> + <Icons.FileOutlined iconSize="l" /> + </span> + <span className="tree-node-title">{data.name}</span> + </div> + ); + } + + // Column nodes use ColumnElement + if (identifier === 'column' && data.columnData) { + return ( + <StyledColumnNode + className="tree-node" + style={style} + data-selected={node.isSelected} + onClick={() => node.select()} + > + <ColumnElement column={data.columnData} /> + </StyledColumnNode> + ); + } + + return ( + <div + className="tree-node" + style={style} + data-selected={node.isSelected} + onClick={e => { + e.stopPropagation(); + if (node.isLeaf) { + node.select(); + } else { + node.toggle(); + } + }} + > + <span + className="tree-node-icon" + css={css` + opacity: ${getOpacity(data.disableCheckbox)}; + `} + > + {renderIcon()} + </span> + <Typography.Text + className="tree-node-title" + css={css` + opacity: ${getOpacity(data.disableCheckbox)}; + `} + ellipsis={{ + tooltip: { title: data.name, placement: 'topLeft' }, + }} + > + {highlightText(data.name, searchTerm)} + </Typography.Text> + {identifier === 'schema' && ( + <div className="side-action-container" role="menu"> + <RefreshLabel + onClick={e => { + e.stopPropagation(); + fetchLazyTables({ + dbId: _dbId, Review Comment: **Suggestion:** `fetchLazyTables` is passed `_dbId` which is a string from `data.id.split(':')`; if the API expects a numeric `dbId` this is a type/logic mismatch and can cause unexpected failures; convert the extracted `_dbId` to a number (or undefined) before passing into `fetchLazyTables`. [type error] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ❌ Force-refresh may send wrong dbId type. - ⚠️ Schema refresh network request may be malformed. - ⚠️ Developer build may show type mismatch warnings. ``` </details> ```suggestion dbId: _dbId ? Number(_dbId) : undefined, ``` <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Open SQLLab and expand a schema node in the TableExploreTree UI (TreeNodeRenderer at superset-frontend/src/SqlLab/components/TableExploreTree/TreeNodeRenderer.tsx). The "Force refresh table list" RefreshLabel's onClick handler is defined at lines 204-215; it calls fetchLazyTables at lines 207-212 with dbId: _dbId. 2. If node.data.id encodes the database id as a numeric string (e.g., "schema:12:public") the local variable `_dbId` is a string (extracted at line 89). Execution reaches the fetchLazyTables call at line 207 passing that string through. 3. If FetchLazyTablesParams (types referenced from ./types) or downstream API consumers strictly expect a number, the API call may fail type checks or perform unexpected behavior (server or runtime error) when it receives a string instead of a number. 4. Reproduce by creating a schema node whose db id must be numeric, click the RefreshLabel, and observe either TypeScript lint warnings (during build) or runtime/behavioural differences in the network request payload or error logs if the backend rejects string ids. ``` </details> <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset-frontend/src/SqlLab/components/TableExploreTree/TreeNodeRenderer.tsx **Line:** 208:208 **Comment:** *Type Error: `fetchLazyTables` is passed `_dbId` which is a string from `data.id.split(':')`; if the API expects a numeric `dbId` this is a type/logic mismatch and can cause unexpected failures; convert the extracted `_dbId` to a number (or undefined) before passing into `fetchLazyTables`. Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. ``` </details> ########## superset-frontend/src/SqlLab/components/SqlEditorLeftBar/index.tsx: ########## @@ -16,154 +16,189 @@ * specific language governing permissions and limitations * under the License. */ -import { useCallback, useMemo, useState } from 'react'; -import { shallowEqual, useDispatch, useSelector } from 'react-redux'; +import { useCallback, useState } from 'react'; +import { useDispatch } from 'react-redux'; -import { SqlLabRootState, Table } from 'src/SqlLab/types'; +import { resetState } from 'src/SqlLab/actions/sqlLab'; import { - addTable, - removeTables, - collapseTable, - expandTable, - resetState, -} from 'src/SqlLab/actions/sqlLab'; -import { Button, EmptyState, Icons } from '@superset-ui/core/components'; + Button, + EmptyState, + Flex, + Icons, + Popover, + Typography, +} from '@superset-ui/core/components'; import { t } from '@apache-superset/core'; import { styled, css } from '@apache-superset/core/ui'; -import { TableSelectorMultiple } from 'src/components/TableSelector'; -import useQueryEditor from 'src/SqlLab/hooks/useQueryEditor'; -import { noop } from 'lodash'; -import TableElement from '../TableElement'; +import type { SchemaOption, CatalogOption } from 'src/hooks/apiResources'; +import { DatabaseSelector, type DatabaseObject } from 'src/components'; + import useDatabaseSelector from '../SqlEditorTopBar/useDatabaseSelector'; +import TableExploreTree from '../TableExploreTree'; export interface SqlEditorLeftBarProps { queryEditorId: string; } -const StyledScrollbarContainer = styled.div` - flex: 1 1 auto; - overflow: auto; -`; - const LeftBarStyles = styled.div` + display: flex; + flex-direction: column; + gap: ${({ theme }) => theme.sizeUnit * 2}px; + ${({ theme }) => css` height: 100%; display: flex; flex-direction: column; .divider { border-bottom: 1px solid ${theme.colorSplit}; - margin: ${theme.sizeUnit * 4}px 0; + margin: ${theme.sizeUnit * 1}px 0; } `} `; +const StyledDivider = styled.div` + border-bottom: 1px solid ${({ theme }) => theme.colorSplit}; + margin: 0 -${({ theme }) => theme.sizeUnit * 2.5}px 0; +`; + const SqlEditorLeftBar = ({ queryEditorId }: SqlEditorLeftBarProps) => { - const { db: userSelectedDb, ...dbSelectorProps } = - useDatabaseSelector(queryEditorId); - const allSelectedTables = useSelector<SqlLabRootState, Table[]>( - ({ sqlLab }) => - sqlLab.tables.filter(table => table.queryEditorId === queryEditorId), - shallowEqual, - ); + const dbSelectorProps = useDatabaseSelector(queryEditorId); + const { db, catalog, schema, onDbChange, onCatalogChange, onSchemaChange } = + dbSelectorProps; + const dispatch = useDispatch(); - const queryEditor = useQueryEditor(queryEditorId, [ - 'dbId', - 'catalog', - 'schema', - 'tabViewId', - ]); - const [_emptyResultsWithSearch, setEmptyResultsWithSearch] = useState(false); - const { dbId, schema } = queryEditor; - const tables = useMemo( - () => - allSelectedTables.filter( - table => table.dbId === dbId && table.schema === schema, - ), - [allSelectedTables, dbId, schema], - ); + const shouldShowReset = window.location.search === '?reset=1'; - noop(_emptyResultsWithSearch); // This is to avoid unused variable warning, can be removed if not needed + // Modal state for Database/Catalog/Schema selector + const [selectorModalOpen, setSelectorModalOpen] = useState(false); + const [modalDb, setModalDb] = useState<DatabaseObject | undefined>(undefined); + const [modalCatalog, setModalCatalog] = useState< + CatalogOption | null | undefined + >(undefined); + const [modalSchema, setModalSchema] = useState<SchemaOption | undefined>( + undefined, + ); - const onEmptyResults = useCallback((searchText?: string) => { - setEmptyResultsWithSearch(!!searchText); + const openSelectorModal = useCallback(() => { + setModalDb(db ?? undefined); + setModalCatalog( + catalog ? { label: catalog, value: catalog, title: catalog } : undefined, + ); + setModalSchema( + schema ? { label: schema, value: schema, title: schema } : undefined, + ); + setSelectorModalOpen(true); + }, [db, catalog, schema]); + + const closeSelectorModal = useCallback(() => { + setSelectorModalOpen(false); }, []); - const selectedTableNames = useMemo( - () => tables?.map(table => table.name) || [], - [tables], - ); - - const onTablesChange = ( - tableNames: string[], - catalogName: string | null, - schemaName: string, - ) => { - if (!schemaName) { - return; + const handleModalOk = useCallback(() => { + if (modalDb && modalDb.id !== db?.id) { + onDbChange?.(modalDb); } - - const currentTables = [...tables]; - const tablesToAdd = tableNames.filter(name => { - const index = currentTables.findIndex(table => table.name === name); - if (index >= 0) { - currentTables.splice(index, 1); - return false; - } - - return true; - }); - - tablesToAdd.forEach(tableName => { - dispatch(addTable(queryEditor, tableName, catalogName, schemaName)); - }); - - dispatch(removeTables(currentTables)); - }; - - const onToggleTable = (updatedTables: string[]) => { - tables.forEach(table => { - if (!updatedTables.includes(table.id.toString()) && table.expanded) { - dispatch(collapseTable(table)); - } else if ( - updatedTables.includes(table.id.toString()) && - !table.expanded - ) { - dispatch(expandTable(table)); - } - }); - }; - - const shouldShowReset = window.location.search === '?reset=1'; + if (modalCatalog?.value !== catalog) { Review Comment: **Suggestion:** `handleModalOk` compares `modalCatalog?.value !== catalog` and then calls `onCatalogChange` with `modalCatalog?.value`; when `modalCatalog` is undefined this comparison becomes `undefined !== catalog` and will unintentionally clear the catalog. Only call `onCatalogChange` when `modalCatalog` is defined and its value actually changed. [logic error] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ❌ Catalog can be unintentionally cleared in SQL editor left bar. - ⚠️ Database selector popover interaction inconsistent. ``` </details> ```suggestion if (modalCatalog && modalCatalog.value !== catalog) { onCatalogChange?.(modalCatalog.value); ``` <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Open src/SqlLab/components/SqlEditorLeftBar/index.tsx and inspect modal state setup: openSelectorModal initializes modalCatalog from current catalog at lines 83-90 in the PR hunk (`setModalCatalog(catalog ? { label: catalog, value: catalog, title: catalog } : undefined)`). 2. The popover content shown to users contains a DatabaseSelector bound to modalCatalog (the popoverContent block, lines ~125-157). If the popover is rendered without openSelectorModal having been invoked (for example, see the Popover onOpenChange handler at lines 183-189 that does not set selectorModalOpen true), modalCatalog may remain undefined when the user presses the Select button. 3. Pressing Select triggers handleModalOk (defined at lines 98-119). With modalCatalog undefined and an existing catalog value, the condition `modalCatalog?.value !== catalog` evaluates to true (undefined !== 'currentCatalog') and the code calls `onCatalogChange?.(modalCatalog?.value)` which passes undefined to the handler (line 103), clearing the catalog unexpectedly. 4. This behaviour can therefore be reproduced by opening the Popover without calling openSelectorModal (so modalCatalog is undefined) and then immediately clicking Select; the current code will clear the catalog. Note: if openSelectorModal was called prior to showing the popover, modalCatalog is initialized and this specific unexpected-clear path does not occur. ``` </details> <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset-frontend/src/SqlLab/components/SqlEditorLeftBar/index.tsx **Line:** 102:102 **Comment:** *Logic Error: `handleModalOk` compares `modalCatalog?.value !== catalog` and then calls `onCatalogChange` with `modalCatalog?.value`; when `modalCatalog` is undefined this comparison becomes `undefined !== catalog` and will unintentionally clear the catalog. Only call `onCatalogChange` when `modalCatalog` is defined and its value actually changed. Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. ``` </details> ########## superset-frontend/src/SqlLab/components/TableExploreTree/index.tsx: ########## @@ -0,0 +1,330 @@ +/** + * 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, + useState, + useRef, + type ChangeEvent, + useMemo, +} from 'react'; +import { useSelector, useDispatch, shallowEqual } from 'react-redux'; +import { styled, t } from '@apache-superset/core'; +import AutoSizer from 'react-virtualized-auto-sizer'; +// Due to performance issues with the virtual list in the existing Ant Design (antd)-based tree view, +// it has been replaced with react-arborist solution. +import { Tree, TreeApi, NodeApi } from 'react-arborist'; +import { + Icons, + Skeleton, + Input, + Empty, + Button, +} from '@superset-ui/core/components'; +import type { SqlLabRootState } from 'src/SqlLab/types'; +import useQueryEditor from 'src/SqlLab/hooks/useQueryEditor'; +import { addTable } from 'src/SqlLab/actions/sqlLab'; +import PanelToolbar from 'src/components/PanelToolbar'; +import { ViewContribution } from 'src/SqlLab/contributions'; +import TreeNodeRenderer from './TreeNodeRenderer'; +import useTreeData, { EMPTY_NODE_ID_PREFIX } from './useTreeData'; +import type { TreeNodeData } from './types'; + +type Props = { + queryEditorId: string; +}; + +const StyledTreeContainer = styled.div` + flex: 1 1 auto; + .tree-node { + display: flex; + align-items: center; + padding: 0 ${({ theme }) => theme.sizeUnit}px; + position: relative; + cursor: pointer; + + &:hover { + background-color: ${({ theme }) => theme.colorBgTextHover}; + + .side-action-container { + opacity: 1; + } + } + + &[data-selected='true'] { + background-color: ${({ theme }) => theme.colorBgTextActive}; + + .side-action-container { + opacity: 1; + } + } + } + + .tree-node-icon { + margin-right: ${({ theme }) => theme.sizeUnit}px; + display: flex; + align-items: center; + gap: ${({ theme }) => theme.sizeUnit}px; + } + + .tree-node-title { + white-space: nowrap; + overflow: hidden; + text-overflow: ellipsis; + flex: 1; + } + + .highlighted { + background-color: ${({ theme }) => theme.colorWarningBg}; + font-weight: bold; + } + + .side-action-container { + opacity: 0; + position: absolute; + right: ${({ theme }) => theme.sizeUnit * 1.5}px; + top: 50%; + transform: translateY(-50%); + z-index: ${({ theme }) => theme.zIndexPopupBase}; + } +`; + +const ROW_HEIGHT = 28; + +const TableExploreTree: React.FC<Props> = ({ queryEditorId }) => { + const dispatch = useDispatch(); + const treeRef = useRef<TreeApi<TreeNodeData>>(null); + const tables = useSelector( + ({ sqlLab }: SqlLabRootState) => sqlLab.tables, + shallowEqual, + ); + const queryEditor = useQueryEditor(queryEditorId, [ + 'dbId', + 'schema', + 'catalog', + ]); + const { dbId, catalog, schema: selectedSchema } = queryEditor; + const pinnedTables = useMemo( + () => + Object.fromEntries( + tables.map(({ queryEditorId, dbId, schema, name, persistData }) => [ + queryEditor.id === queryEditorId ? `${dbId}:${schema}:${name}` : '', + persistData, + ]), + ), + [tables, queryEditor.id], + ); + + // Tree data hook - manages schema/table/column data fetching and tree structure + const { + treeData, + isFetching, + refetch, + loadingNodes, + handleToggle, + fetchLazyTables, + } = useTreeData({ + dbId, + catalog, + selectedSchema, + pinnedTables, + }); + + const handlePinTable = useCallback( + (tableName: string, schemaName: string, catalogName: string | null) => + dispatch(addTable(queryEditor, tableName, catalogName, schemaName)), + [dispatch, queryEditor], + ); + const [searchTerm, setSearchTerm] = useState(''); + const handleSearchChange = useCallback( + ({ target }: ChangeEvent<HTMLInputElement>) => setSearchTerm(target.value), + [], + ); + + // Track manually opened nodes (not auto-expanded by search) + const [manuallyOpenedNodes, setManuallyOpenedNodes] = useState< + Record<string, boolean> + >({}); + + // Custom search match function for react-arborist + const searchMatch = useCallback( + (node: NodeApi<TreeNodeData>, term: string): boolean => { + // Empty placeholder nodes should not match search + if (node.data.type === 'empty') return false; + if (!term) return true; + + const lowerTerm = term.toLowerCase(); + + // Check if current node matches + if (node.data.name.toLowerCase().includes(lowerTerm)) { + return true; + } + + // Check if any ancestor matches - if so, include this node (child of matching parent) + // if (node.parent && node.parent.isRoot === false && node.parent.isOpen) { + // return true; + // } + let ancestor = node.parent; + while (ancestor && !ancestor.isRoot) { + if (ancestor.data.name.toLowerCase().includes(lowerTerm)) { Review Comment: **Suggestion:** The ancestor-matching loop in `searchMatch` stops before checking the root ancestor (it uses `while (ancestor && !ancestor.isRoot)`), so if the root node matches the search term its children won't be included; also there's no guard for missing `name` on ancestor data. Iterate while `ancestor` exists and safely check `ancestor.data.name`. [logic error] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ❌ Search omits matching children in TableExploreTree. - ⚠️ Users see "No matching results" incorrectly. ``` </details> ```suggestion // Check if any ancestor matches - include root as well and guard against missing names let ancestor = node.parent; while (ancestor) { const ancestorName = ancestor.data && (ancestor.data as any).name; if (typeof ancestorName === 'string' && ancestorName.toLowerCase().includes(lowerTerm)) { ``` <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. In the running app, open the TableExploreTree UI (file: superset-frontend/src/SqlLab/components/TableExploreTree/index.tsx). The search input is rendered at lines 262-269 and updates searchTerm via handleSearchChange. 2. Type a search term that exactly matches the name of a parent/ancestor node (for example, a schema name present in the tree data produced by useTreeData at lines 134-146). This sets searchTerm and causes the Tree component to receive searchTerm and searchMatch (Tree props at lines 287-296). 3. React-arborist calls the provided searchMatch (defined at lines 165-192). The current ancestor loop uses while (ancestor && !ancestor.isRoot), which stops iterating before checking an ancestor whose isRoot is true. If the matching ancestor has isRoot === true (a top-level node in this tree shape), its name is never checked and child nodes are not included. 4. The user-visible result: a search that should include a parent's children is treated as no-match. The component then renders the Empty state when hasMatchingNodes is false (Empty rendered at lines 277-284), causing missing search results for legitimate matches. ``` </details> <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset-frontend/src/SqlLab/components/TableExploreTree/index.tsx **Line:** 178:184 **Comment:** *Logic Error: The ancestor-matching loop in `searchMatch` stops before checking the root ancestor (it uses `while (ancestor && !ancestor.isRoot)`), so if the root node matches the search term its children won't be included; also there's no guard for missing `name` on ancestor data. Iterate while `ancestor` exists and safely check `ancestor.data.name`. Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. ``` </details> ########## superset-frontend/src/components/DatabaseSelector/index.tsx: ########## @@ -436,7 +565,22 @@ export function DatabaseSelector({ } return ( - <DatabaseSelectorWrapper data-test="DatabaseSelector"> + <DatabaseSelectorWrapper + data-test="DatabaseSelector" + horizontal={Boolean(sqlLabMode)} + onClick={sqlLabMode ? onOpenModal : undefined} + role={sqlLabMode ? 'button' : undefined} + tabIndex={sqlLabMode ? 0 : undefined} + onKeyDown={ + sqlLabMode + ? e => { + if (e.key === 'Enter' || e.key === ' ') { Review Comment: **Suggestion:** The keyboard handler only checks for e.key === ' ' to detect the Space key, but some browsers/reporting use 'Spacebar' or provide e.code === 'Space'; this can cause the Space key to fail to open the modal — update the condition to include common variants and e.code for reliable detection. [possible bug] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ❌ Keyboard Space may not open SQLLab selector modal. - ⚠️ Accessibility: inconsistent keyboard activation behavior. ``` </details> ```suggestion const isEnter = e.key === 'Enter'; const isSpaceKey = e.key === ' ' || e.key === 'Spacebar' || e.code === 'Space'; if (isEnter || isSpaceKey) { onOpenModal?.(); // prevent default for space to avoid page scrolling where appropriate e.preventDefault(); ``` <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Mount DatabaseSelector with sqlLabMode=true so the component returns the DatabaseSelectorWrapper configured as an interactive control (see return wrapper at new hunk lines ~568-583 in the PR). 2. Focus the wrapper (it is keyboard-focusable because tabIndex is set at the wrapper: new hunk line ~573). This is the documented keyboard entry point for opening the modal. 3. Press the Space key. The existing handler at onKeyDown (new hunk lines 574-581) only checks `e.key === ' '` and `e.key === 'Enter'`. Some browser/agent combinations may report the Space key as 'Spacebar' or provide e.code==='Space' (legacy/variant values), so the condition can fail and the handler will not call onOpenModal. 4. Observe outcome: Enter will still open the modal but Space may do nothing or produce default page scrolling instead of activating the control. Because the code does not call e.preventDefault() for Space, pressing Space can scroll the page rather than open the modal. ``` </details> <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset-frontend/src/components/DatabaseSelector/index.tsx **Line:** 577:577 **Comment:** *Possible Bug: The keyboard handler only checks for e.key === ' ' to detect the Space key, but some browsers/reporting use 'Spacebar' or provide e.code === 'Space'; this can cause the Space key to fail to open the modal — update the condition to include common variants and e.code for reliable detection. Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. ``` </details> ########## superset-frontend/src/SqlLab/components/TableExploreTree/index.tsx: ########## @@ -0,0 +1,330 @@ +/** + * 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, + useState, + useRef, + type ChangeEvent, + useMemo, +} from 'react'; +import { useSelector, useDispatch, shallowEqual } from 'react-redux'; +import { styled, t } from '@apache-superset/core'; +import AutoSizer from 'react-virtualized-auto-sizer'; +// Due to performance issues with the virtual list in the existing Ant Design (antd)-based tree view, +// it has been replaced with react-arborist solution. +import { Tree, TreeApi, NodeApi } from 'react-arborist'; +import { + Icons, + Skeleton, + Input, + Empty, + Button, +} from '@superset-ui/core/components'; +import type { SqlLabRootState } from 'src/SqlLab/types'; +import useQueryEditor from 'src/SqlLab/hooks/useQueryEditor'; +import { addTable } from 'src/SqlLab/actions/sqlLab'; +import PanelToolbar from 'src/components/PanelToolbar'; +import { ViewContribution } from 'src/SqlLab/contributions'; +import TreeNodeRenderer from './TreeNodeRenderer'; +import useTreeData, { EMPTY_NODE_ID_PREFIX } from './useTreeData'; +import type { TreeNodeData } from './types'; + +type Props = { + queryEditorId: string; +}; + +const StyledTreeContainer = styled.div` + flex: 1 1 auto; + .tree-node { + display: flex; + align-items: center; + padding: 0 ${({ theme }) => theme.sizeUnit}px; + position: relative; + cursor: pointer; + + &:hover { + background-color: ${({ theme }) => theme.colorBgTextHover}; + + .side-action-container { + opacity: 1; + } + } + + &[data-selected='true'] { + background-color: ${({ theme }) => theme.colorBgTextActive}; + + .side-action-container { + opacity: 1; + } + } + } + + .tree-node-icon { + margin-right: ${({ theme }) => theme.sizeUnit}px; + display: flex; + align-items: center; + gap: ${({ theme }) => theme.sizeUnit}px; + } + + .tree-node-title { + white-space: nowrap; + overflow: hidden; + text-overflow: ellipsis; + flex: 1; + } + + .highlighted { + background-color: ${({ theme }) => theme.colorWarningBg}; + font-weight: bold; + } + + .side-action-container { + opacity: 0; + position: absolute; + right: ${({ theme }) => theme.sizeUnit * 1.5}px; + top: 50%; + transform: translateY(-50%); + z-index: ${({ theme }) => theme.zIndexPopupBase}; + } +`; + +const ROW_HEIGHT = 28; + +const TableExploreTree: React.FC<Props> = ({ queryEditorId }) => { + const dispatch = useDispatch(); + const treeRef = useRef<TreeApi<TreeNodeData>>(null); + const tables = useSelector( + ({ sqlLab }: SqlLabRootState) => sqlLab.tables, + shallowEqual, + ); + const queryEditor = useQueryEditor(queryEditorId, [ + 'dbId', + 'schema', + 'catalog', + ]); + const { dbId, catalog, schema: selectedSchema } = queryEditor; Review Comment: **Suggestion:** Logic bug when building `pinnedTables`: the current mapping produces entries with an empty-string key for every table that doesn't belong to the active query editor, which results in a spurious '' key and collisions (last-writer wins). Filter the tables first so only matching tables produce entries with the expected composite key. [logic error] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ❌ Pinned tables state incorrect in TableExploreTree component. - ⚠️ useTreeData receives malformed pinnedTables mapping. ``` </details> ```suggestion tables .filter(({ queryEditorId }) => queryEditor.id === queryEditorId) .map(({ dbId, schema, name, persistData }) => [ `${dbId}:${schema}:${name}`, ``` <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Open the TableExploreTree component (file: superset-frontend/src/SqlLab/components/TableExploreTree/index.tsx). The component selects the global tables array via useSelector (around lines 110-115) and obtains the active query editor via useQueryEditor (around lines 116-121). 2. Ensure the Redux state sqlLab.tables contains more than one entry where some entries have queryEditorId !== the active queryEditor.id (this is common when multiple SQL editors have pinned tables). The tables array is the exact source consumed on lines 112-115. 3. When the component renders, the pinnedTables useMemo (lines 117-126) executes. Because the code maps every table to a [key, value] pair and emits an empty string ('') as the key for non-matching queryEditorId, Object.fromEntries will create a pinnedTables[''] entry (and subsequent non-matching rows will overwrite that same '' key — last-writer wins). 4. That pinnedTables object is passed into useTreeData (call site at lines 134-146). The downstream logic expecting keys of the form `${dbId}:${schema}:${name}` will see a spurious '' key, causing pinned state lookups/merges to behave incorrectly (e.g., wrong persisted data applied or missing pinned state for intended tables). The observed symptom in UI: pinned tables appear incorrect in the left sidebar tree. ``` </details> <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset-frontend/src/SqlLab/components/TableExploreTree/index.tsx **Line:** 120:121 **Comment:** *Logic Error: Logic bug when building `pinnedTables`: the current mapping produces entries with an empty-string key for every table that doesn't belong to the active query editor, which results in a spurious '' key and collisions (last-writer wins). Filter the tables first so only matching tables produce entries with the expected composite key. Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. ``` </details> ########## superset-frontend/src/SqlLab/components/TableExploreTree/useTreeData.ts: ########## @@ -0,0 +1,325 @@ +/** + * 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 { useMemo, useReducer, useCallback } from 'react'; +import { t } from '@apache-superset/core'; +import { + Table, + type TableMetaData, + useSchemas, + useLazyTablesQuery, + useLazyTableMetadataQuery, + useLazyTableExtendedMetadataQuery, +} from 'src/hooks/apiResources'; +import type { TreeNodeData } from './types'; + +export const EMPTY_NODE_ID_PREFIX = 'empty:'; + +// Reducer state and actions +interface TreeDataState { + tableData: Record<string, { options: Table[] }>; + tableSchemaData: Record<string, TableMetaData>; + loadingNodes: Record<string, boolean>; +} + +type TreeDataAction = + | { type: 'SET_TABLE_DATA'; key: string; data: { options: Table[] } } + | { type: 'SET_TABLE_SCHEMA_DATA'; key: string; data: TableMetaData } + | { type: 'SET_LOADING_NODE'; nodeId: string; loading: boolean }; + +const initialState: TreeDataState = { + tableData: {}, + tableSchemaData: {}, + loadingNodes: {}, +}; + +function treeDataReducer( + state: TreeDataState, + action: TreeDataAction, +): TreeDataState { + switch (action.type) { + case 'SET_TABLE_DATA': + return { + ...state, + tableData: { ...state.tableData, [action.key]: action.data }, + // Force tree re-render to apply search filter to new data + // dataVersion: state.dataVersion + 1, + }; + case 'SET_TABLE_SCHEMA_DATA': + return { + ...state, + tableSchemaData: { + ...state.tableSchemaData, + [action.key]: action.data, + }, + // Force tree re-render to apply search filter to new data + // dataVersion: state.dataVersion + 1, + }; + case 'SET_LOADING_NODE': + return { + ...state, + loadingNodes: { + ...state.loadingNodes, + [action.nodeId]: action.loading, + }, + }; + default: + return state; + } +} + +interface UseTreeDataParams { + dbId: number | undefined; + catalog: string | null | undefined; + selectedSchema: string | undefined; + pinnedTables: Record<string, TableMetaData | undefined>; +} + +interface UseTreeDataResult { + treeData: TreeNodeData[]; + isFetching: boolean; + refetch: () => void; + loadingNodes: Record<string, boolean>; + dataVersion: number; Review Comment: **Suggestion:** Interface/return mismatch: `UseTreeDataResult` declares `dataVersion: number` but the hook does not provide `dataVersion`, causing a logical type/contract mismatch; remove `dataVersion` from the interface to match the actual returned object or implement `dataVersion` in state and return it. The minimal fix here is to remove the unused `dataVersion` field from the interface. [logic error] <details> <summary><b>Severity Level:</b> Critical 🚨</summary> ```mdx - ❌ TypeScript build fails blocking frontend compilation. - ❌ CI type-check job will fail on this file. - ⚠️ Developers cannot import/use hook without type errors. ``` </details> ```suggestion ``` <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. The hook `useTreeData` is declared with an explicit return type `: UseTreeDataResult` (function signature at line 109 in useTreeData.ts). The interface `UseTreeDataResult` currently includes `dataVersion: number` (definition lines 93-101). 2. The implementation returns an object literal at lines 315-322 which does NOT include `dataVersion`. TypeScript therefore reports a type mismatch: the implementation's return value is missing a required property declared in the return interface. This is a compile-time / type-check error. 3. Reproduce by running the TypeScript build or type-check step (e.g., yarn build or tsc) for the frontend package; the compiler will flag: "Type '{ ... }' is not assignable to type 'UseTreeDataResult'. Property 'dataVersion' is missing." The file paths and lines to inspect are: interface (useTreeData.ts lines 93-101) and the return object (useTreeData.ts lines 315-322). 4. Minimal corrective options: remove `dataVersion` from `UseTreeDataResult` (if unused) or implement and return a `dataVersion` value from the hook's state. Removing it aligns the interface with the actual returned object. ``` </details> <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset-frontend/src/SqlLab/components/TableExploreTree/useTreeData.ts **Line:** 98:98 **Comment:** *Logic Error: Interface/return mismatch: `UseTreeDataResult` declares `dataVersion: number` but the hook does not provide `dataVersion`, causing a logical type/contract mismatch; remove `dataVersion` from the interface to match the actual returned object or implement `dataVersion` in state and return it. The minimal fix here is to remove the unused `dataVersion` field from the interface. Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. ``` </details> ########## superset-frontend/src/SqlLab/components/SqlEditorLeftBar/index.tsx: ########## @@ -16,154 +16,189 @@ * specific language governing permissions and limitations * under the License. */ -import { useCallback, useMemo, useState } from 'react'; -import { shallowEqual, useDispatch, useSelector } from 'react-redux'; +import { useCallback, useState } from 'react'; +import { useDispatch } from 'react-redux'; -import { SqlLabRootState, Table } from 'src/SqlLab/types'; +import { resetState } from 'src/SqlLab/actions/sqlLab'; import { - addTable, - removeTables, - collapseTable, - expandTable, - resetState, -} from 'src/SqlLab/actions/sqlLab'; -import { Button, EmptyState, Icons } from '@superset-ui/core/components'; + Button, + EmptyState, + Flex, + Icons, + Popover, + Typography, +} from '@superset-ui/core/components'; import { t } from '@apache-superset/core'; import { styled, css } from '@apache-superset/core/ui'; -import { TableSelectorMultiple } from 'src/components/TableSelector'; -import useQueryEditor from 'src/SqlLab/hooks/useQueryEditor'; -import { noop } from 'lodash'; -import TableElement from '../TableElement'; +import type { SchemaOption, CatalogOption } from 'src/hooks/apiResources'; +import { DatabaseSelector, type DatabaseObject } from 'src/components'; + import useDatabaseSelector from '../SqlEditorTopBar/useDatabaseSelector'; +import TableExploreTree from '../TableExploreTree'; export interface SqlEditorLeftBarProps { queryEditorId: string; } -const StyledScrollbarContainer = styled.div` - flex: 1 1 auto; - overflow: auto; -`; - const LeftBarStyles = styled.div` + display: flex; + flex-direction: column; + gap: ${({ theme }) => theme.sizeUnit * 2}px; + ${({ theme }) => css` height: 100%; display: flex; flex-direction: column; .divider { border-bottom: 1px solid ${theme.colorSplit}; - margin: ${theme.sizeUnit * 4}px 0; + margin: ${theme.sizeUnit * 1}px 0; } `} `; +const StyledDivider = styled.div` + border-bottom: 1px solid ${({ theme }) => theme.colorSplit}; + margin: 0 -${({ theme }) => theme.sizeUnit * 2.5}px 0; +`; + const SqlEditorLeftBar = ({ queryEditorId }: SqlEditorLeftBarProps) => { - const { db: userSelectedDb, ...dbSelectorProps } = - useDatabaseSelector(queryEditorId); - const allSelectedTables = useSelector<SqlLabRootState, Table[]>( - ({ sqlLab }) => - sqlLab.tables.filter(table => table.queryEditorId === queryEditorId), - shallowEqual, - ); + const dbSelectorProps = useDatabaseSelector(queryEditorId); + const { db, catalog, schema, onDbChange, onCatalogChange, onSchemaChange } = + dbSelectorProps; + const dispatch = useDispatch(); - const queryEditor = useQueryEditor(queryEditorId, [ - 'dbId', - 'catalog', - 'schema', - 'tabViewId', - ]); - const [_emptyResultsWithSearch, setEmptyResultsWithSearch] = useState(false); - const { dbId, schema } = queryEditor; - const tables = useMemo( - () => - allSelectedTables.filter( - table => table.dbId === dbId && table.schema === schema, - ), - [allSelectedTables, dbId, schema], - ); + const shouldShowReset = window.location.search === '?reset=1'; - noop(_emptyResultsWithSearch); // This is to avoid unused variable warning, can be removed if not needed + // Modal state for Database/Catalog/Schema selector + const [selectorModalOpen, setSelectorModalOpen] = useState(false); + const [modalDb, setModalDb] = useState<DatabaseObject | undefined>(undefined); + const [modalCatalog, setModalCatalog] = useState< + CatalogOption | null | undefined + >(undefined); + const [modalSchema, setModalSchema] = useState<SchemaOption | undefined>( + undefined, + ); - const onEmptyResults = useCallback((searchText?: string) => { - setEmptyResultsWithSearch(!!searchText); + const openSelectorModal = useCallback(() => { + setModalDb(db ?? undefined); + setModalCatalog( + catalog ? { label: catalog, value: catalog, title: catalog } : undefined, + ); + setModalSchema( + schema ? { label: schema, value: schema, title: schema } : undefined, + ); + setSelectorModalOpen(true); + }, [db, catalog, schema]); + + const closeSelectorModal = useCallback(() => { + setSelectorModalOpen(false); }, []); - const selectedTableNames = useMemo( - () => tables?.map(table => table.name) || [], - [tables], - ); - - const onTablesChange = ( - tableNames: string[], - catalogName: string | null, - schemaName: string, - ) => { - if (!schemaName) { - return; + const handleModalOk = useCallback(() => { + if (modalDb && modalDb.id !== db?.id) { + onDbChange?.(modalDb); } - - const currentTables = [...tables]; - const tablesToAdd = tableNames.filter(name => { - const index = currentTables.findIndex(table => table.name === name); - if (index >= 0) { - currentTables.splice(index, 1); - return false; - } - - return true; - }); - - tablesToAdd.forEach(tableName => { - dispatch(addTable(queryEditor, tableName, catalogName, schemaName)); - }); - - dispatch(removeTables(currentTables)); - }; - - const onToggleTable = (updatedTables: string[]) => { - tables.forEach(table => { - if (!updatedTables.includes(table.id.toString()) && table.expanded) { - dispatch(collapseTable(table)); - } else if ( - updatedTables.includes(table.id.toString()) && - !table.expanded - ) { - dispatch(expandTable(table)); - } - }); - }; - - const shouldShowReset = window.location.search === '?reset=1'; + if (modalCatalog?.value !== catalog) { + onCatalogChange?.(modalCatalog?.value); + } + if (modalSchema?.value !== schema) { Review Comment: **Suggestion:** `handleModalOk` calls `onSchemaChange` with `modalSchema?.value ?? ''` whenever `modalSchema?.value !== schema`; if `modalSchema` is undefined this will compare undefined !== schema and may call `onSchemaChange('')` unintentionally clearing the schema. Only call `onSchemaChange` when `modalSchema` exists and its value changed, and pass the actual value. [logic error] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ❌ Schema can be unintentionally cleared in SQL editor left bar. - ⚠️ Database/schema selection UX may confuse users. ``` </details> ```suggestion if (modalSchema && modalSchema.value !== schema) { onSchemaChange?.(modalSchema.value); ``` <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Inspect src/SqlLab/components/SqlEditorLeftBar/index.tsx: openSelectorModal sets modalSchema from schema at lines 83-91; the popoverContent DatabaseSelector uses modalSchema (lines ~137-156). 2. If the Popover is shown without openSelectorModal having initialized modalSchema (for example due to the Popover onOpenChange handler at lines 183-189 not setting selectorModalOpen), modalSchema will be undefined while the component-level schema may still hold a value. 3. When the user clicks the Select button inside the popover, handleModalOk (lines 98-119) runs. The expression `modalSchema?.value !== schema` is true for undefined !== 'currentSchema', and the code calls `onSchemaChange?.(modalSchema?.value ?? '')` which passes the empty string '' (line 106). That results in the schema being set to '' (cleared) unintentionally. 4. This reproduces by opening the popover with uninitialized modal state and immediately clicking Select; the schema will be cleared. If openSelectorModal initialized modalSchema correctly before showing the popover, this path does not occur. ``` </details> <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset-frontend/src/SqlLab/components/SqlEditorLeftBar/index.tsx **Line:** 105:105 **Comment:** *Logic Error: `handleModalOk` calls `onSchemaChange` with `modalSchema?.value ?? ''` whenever `modalSchema?.value !== schema`; if `modalSchema` is undefined this will compare undefined !== schema and may call `onSchemaChange('')` unintentionally clearing the schema. Only call `onSchemaChange` when `modalSchema` exists and its value changed, and pass the actual value. Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. ``` </details> ########## superset-frontend/src/SqlLab/components/SqlEditorLeftBar/index.tsx: ########## @@ -16,154 +16,189 @@ * specific language governing permissions and limitations * under the License. */ -import { useCallback, useMemo, useState } from 'react'; -import { shallowEqual, useDispatch, useSelector } from 'react-redux'; +import { useCallback, useState } from 'react'; +import { useDispatch } from 'react-redux'; -import { SqlLabRootState, Table } from 'src/SqlLab/types'; +import { resetState } from 'src/SqlLab/actions/sqlLab'; import { - addTable, - removeTables, - collapseTable, - expandTable, - resetState, -} from 'src/SqlLab/actions/sqlLab'; -import { Button, EmptyState, Icons } from '@superset-ui/core/components'; + Button, + EmptyState, + Flex, + Icons, + Popover, + Typography, +} from '@superset-ui/core/components'; import { t } from '@apache-superset/core'; import { styled, css } from '@apache-superset/core/ui'; -import { TableSelectorMultiple } from 'src/components/TableSelector'; -import useQueryEditor from 'src/SqlLab/hooks/useQueryEditor'; -import { noop } from 'lodash'; -import TableElement from '../TableElement'; +import type { SchemaOption, CatalogOption } from 'src/hooks/apiResources'; +import { DatabaseSelector, type DatabaseObject } from 'src/components'; + import useDatabaseSelector from '../SqlEditorTopBar/useDatabaseSelector'; +import TableExploreTree from '../TableExploreTree'; export interface SqlEditorLeftBarProps { queryEditorId: string; } -const StyledScrollbarContainer = styled.div` - flex: 1 1 auto; - overflow: auto; -`; - const LeftBarStyles = styled.div` + display: flex; + flex-direction: column; + gap: ${({ theme }) => theme.sizeUnit * 2}px; + ${({ theme }) => css` height: 100%; display: flex; flex-direction: column; .divider { border-bottom: 1px solid ${theme.colorSplit}; - margin: ${theme.sizeUnit * 4}px 0; + margin: ${theme.sizeUnit * 1}px 0; } `} `; +const StyledDivider = styled.div` + border-bottom: 1px solid ${({ theme }) => theme.colorSplit}; + margin: 0 -${({ theme }) => theme.sizeUnit * 2.5}px 0; +`; + const SqlEditorLeftBar = ({ queryEditorId }: SqlEditorLeftBarProps) => { - const { db: userSelectedDb, ...dbSelectorProps } = - useDatabaseSelector(queryEditorId); - const allSelectedTables = useSelector<SqlLabRootState, Table[]>( - ({ sqlLab }) => - sqlLab.tables.filter(table => table.queryEditorId === queryEditorId), - shallowEqual, - ); + const dbSelectorProps = useDatabaseSelector(queryEditorId); + const { db, catalog, schema, onDbChange, onCatalogChange, onSchemaChange } = + dbSelectorProps; + const dispatch = useDispatch(); - const queryEditor = useQueryEditor(queryEditorId, [ - 'dbId', - 'catalog', - 'schema', - 'tabViewId', - ]); - const [_emptyResultsWithSearch, setEmptyResultsWithSearch] = useState(false); - const { dbId, schema } = queryEditor; - const tables = useMemo( - () => - allSelectedTables.filter( - table => table.dbId === dbId && table.schema === schema, - ), - [allSelectedTables, dbId, schema], - ); + const shouldShowReset = window.location.search === '?reset=1'; - noop(_emptyResultsWithSearch); // This is to avoid unused variable warning, can be removed if not needed + // Modal state for Database/Catalog/Schema selector + const [selectorModalOpen, setSelectorModalOpen] = useState(false); + const [modalDb, setModalDb] = useState<DatabaseObject | undefined>(undefined); + const [modalCatalog, setModalCatalog] = useState< + CatalogOption | null | undefined + >(undefined); + const [modalSchema, setModalSchema] = useState<SchemaOption | undefined>( + undefined, + ); - const onEmptyResults = useCallback((searchText?: string) => { - setEmptyResultsWithSearch(!!searchText); + const openSelectorModal = useCallback(() => { + setModalDb(db ?? undefined); + setModalCatalog( + catalog ? { label: catalog, value: catalog, title: catalog } : undefined, + ); + setModalSchema( + schema ? { label: schema, value: schema, title: schema } : undefined, + ); + setSelectorModalOpen(true); + }, [db, catalog, schema]); + + const closeSelectorModal = useCallback(() => { + setSelectorModalOpen(false); }, []); - const selectedTableNames = useMemo( - () => tables?.map(table => table.name) || [], - [tables], - ); - - const onTablesChange = ( - tableNames: string[], - catalogName: string | null, - schemaName: string, - ) => { - if (!schemaName) { - return; + const handleModalOk = useCallback(() => { + if (modalDb && modalDb.id !== db?.id) { + onDbChange?.(modalDb); } - - const currentTables = [...tables]; - const tablesToAdd = tableNames.filter(name => { - const index = currentTables.findIndex(table => table.name === name); - if (index >= 0) { - currentTables.splice(index, 1); - return false; - } - - return true; - }); - - tablesToAdd.forEach(tableName => { - dispatch(addTable(queryEditor, tableName, catalogName, schemaName)); - }); - - dispatch(removeTables(currentTables)); - }; - - const onToggleTable = (updatedTables: string[]) => { - tables.forEach(table => { - if (!updatedTables.includes(table.id.toString()) && table.expanded) { - dispatch(collapseTable(table)); - } else if ( - updatedTables.includes(table.id.toString()) && - !table.expanded - ) { - dispatch(expandTable(table)); - } - }); - }; - - const shouldShowReset = window.location.search === '?reset=1'; + if (modalCatalog?.value !== catalog) { + onCatalogChange?.(modalCatalog?.value); + } + if (modalSchema?.value !== schema) { + onSchemaChange?.(modalSchema?.value ?? ''); + } + setSelectorModalOpen(false); + }, [ + modalDb, + modalCatalog, + modalSchema, + db, + catalog, + schema, + onDbChange, + onCatalogChange, + onSchemaChange, + ]); const handleResetState = useCallback(() => { dispatch(resetState()); }, [dispatch]); - return ( - <LeftBarStyles data-test="sql-editor-left-bar"> - <TableSelectorMultiple - {...dbSelectorProps} - onEmptyResults={onEmptyResults} + const popoverContent = ( + <Flex + vertical + gap="middle" + data-test="DatabaseSelector" + css={css` + min-width: 500px; + `} + > + <Typography.Title level={5} style={{ margin: 0 }}> + {t('Select Database and Schema')} + </Typography.Title> + <DatabaseSelector + key={modalDb ? modalDb.id : 'no-db'} + db={modalDb} emptyState={<EmptyState />} - database={userSelectedDb} - onTableSelectChange={onTablesChange} - tableValue={selectedTableNames} - sqlLabMode + getDbList={dbSelectorProps.getDbList} + handleError={dbSelectorProps.handleError} + onDbChange={setModalDb} + onCatalogChange={cat => + setModalCatalog( + cat ? { label: cat, value: cat, title: cat } : undefined, + ) + } + catalog={modalCatalog?.value} + onSchemaChange={sch => + setModalSchema( + sch ? { label: sch, value: sch, title: sch } : undefined, + ) + } + schema={modalSchema?.value} + sqlLabMode={false} /> - <div className="divider" /> - <StyledScrollbarContainer> - {tables.map(table => ( - <TableElement - table={table} - key={table.id} - activeKey={tables - .filter(({ expanded }) => expanded) - .map(({ id }) => id)} - onChange={onToggleTable} - /> - ))} - </StyledScrollbarContainer> + <Flex justify="flex-end" gap="small"> + <Button + buttonStyle="tertiary" + onClick={e => { + e?.stopPropagation(); + closeSelectorModal(); + }} + > + {t('Cancel')} + </Button> + <Button + type="primary" + onClick={e => { + e?.stopPropagation(); + handleModalOk(); + }} + > + {t('Select')} + </Button> + </Flex> + </Flex> + ); + + return ( + <LeftBarStyles data-test="sql-editor-left-bar"> + <Popover + content={popoverContent} + open={selectorModalOpen} + onOpenChange={open => !open && closeSelectorModal()} Review Comment: **Suggestion:** The Popover's onOpenChange handler ignores the provided open state and only calls close when open is false; it doesn't update the controlled `selectorModalOpen` state when the Popover requests to open, causing the Popover UI and internal state to get out of sync. Use the open boolean to set the state directly. [logic error] <details> <summary><b>Severity Level:</b> Critical 🚨</summary> ```mdx - ❌ Database selector popover fails to open in SQL editor. - ⚠️ User cannot reliably change DB/catalog/schema from left bar. ``` </details> ```suggestion onOpenChange={open => setSelectorModalOpen(open)} ``` <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Open the SQL editor UI and locate the left bar component in the app. The component is implemented in src/SqlLab/components/SqlEditorLeftBar/index.tsx (SqlEditorLeftBar component). 2. In that file, the Popover wrapper around the DatabaseSelector is defined at lines 183-189; it is a controlled Popover using the selectorModalOpen state (see lines 181-190 in the PR diff). The onOpenChange handler is currently `onOpenChange={open => !open && closeSelectorModal()}` (line 186 in the PR hunk). 3. Click the DatabaseSelector UI in the left bar (the element wrapped by the Popover). The Popover component will invoke onOpenChange with open === true when it wants to open, but the current handler ignores the open === true case and does not set selectorModalOpen to true (it only calls closeSelectorModal on close). Because `selectorModalOpen` remains false, the Popover will remain closed or get out of sync with user interaction. 4. Expected behaviour: clicking the trigger should open the Popover. Observed behaviour with current code: Popover does not reliably open when the trigger is clicked (or its internal open state and the component's selectorModalOpen diverge), causing the Database selection UI to not appear or to be inconsistently visible. ``` </details> <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset-frontend/src/SqlLab/components/SqlEditorLeftBar/index.tsx **Line:** 186:186 **Comment:** *Logic Error: The Popover's onOpenChange handler ignores the provided open state and only calls close when open is false; it doesn't update the controlled `selectorModalOpen` state when the Popover requests to open, causing the Popover UI and internal state to get out of sync. Use the open boolean to set the state directly. Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. ``` </details> ########## superset-frontend/src/SqlLab/components/TableExploreTree/useTreeData.ts: ########## @@ -0,0 +1,325 @@ +/** + * 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 { useMemo, useReducer, useCallback } from 'react'; +import { t } from '@apache-superset/core'; +import { + Table, + type TableMetaData, + useSchemas, + useLazyTablesQuery, + useLazyTableMetadataQuery, + useLazyTableExtendedMetadataQuery, +} from 'src/hooks/apiResources'; +import type { TreeNodeData } from './types'; + +export const EMPTY_NODE_ID_PREFIX = 'empty:'; + +// Reducer state and actions +interface TreeDataState { + tableData: Record<string, { options: Table[] }>; + tableSchemaData: Record<string, TableMetaData>; + loadingNodes: Record<string, boolean>; +} + +type TreeDataAction = + | { type: 'SET_TABLE_DATA'; key: string; data: { options: Table[] } } + | { type: 'SET_TABLE_SCHEMA_DATA'; key: string; data: TableMetaData } + | { type: 'SET_LOADING_NODE'; nodeId: string; loading: boolean }; + +const initialState: TreeDataState = { + tableData: {}, + tableSchemaData: {}, + loadingNodes: {}, +}; + +function treeDataReducer( + state: TreeDataState, + action: TreeDataAction, +): TreeDataState { + switch (action.type) { + case 'SET_TABLE_DATA': + return { + ...state, + tableData: { ...state.tableData, [action.key]: action.data }, + // Force tree re-render to apply search filter to new data + // dataVersion: state.dataVersion + 1, + }; + case 'SET_TABLE_SCHEMA_DATA': + return { + ...state, + tableSchemaData: { + ...state.tableSchemaData, + [action.key]: action.data, + }, + // Force tree re-render to apply search filter to new data + // dataVersion: state.dataVersion + 1, + }; + case 'SET_LOADING_NODE': + return { + ...state, + loadingNodes: { + ...state.loadingNodes, + [action.nodeId]: action.loading, + }, + }; + default: + return state; + } +} + +interface UseTreeDataParams { + dbId: number | undefined; + catalog: string | null | undefined; + selectedSchema: string | undefined; + pinnedTables: Record<string, TableMetaData | undefined>; +} + +interface UseTreeDataResult { + treeData: TreeNodeData[]; + isFetching: boolean; + refetch: () => void; + loadingNodes: Record<string, boolean>; + dataVersion: number; + handleToggle: (id: string, isOpen: boolean) => Promise<void>; + fetchLazyTables: ReturnType<typeof useLazyTablesQuery>[0]; +} + +const createEmptyNode = (parentId: string): TreeNodeData => ({ + id: `${EMPTY_NODE_ID_PREFIX}${parentId}`, + name: t('No items'), + type: 'empty', +}); + +const useTreeData = ({ + dbId, + catalog, + selectedSchema, + pinnedTables, +}: UseTreeDataParams): UseTreeDataResult => { + // Schema data from API + const { + currentData: schemaData, + isFetching, + refetch, + } = useSchemas({ dbId, catalog: catalog || undefined }); + // Lazy query hooks + const [fetchLazyTables] = useLazyTablesQuery(); + const [fetchTableMetadata] = useLazyTableMetadataQuery(); + const [fetchTableExtendedMetadata] = useLazyTableExtendedMetadataQuery(); + + // Combined state for table data, schema data, loading nodes, and data version + const [state, dispatch] = useReducer(treeDataReducer, initialState); + const { tableData, tableSchemaData, loadingNodes } = state; + + // Handle async loading when node is toggled open + const handleToggle = useCallback( + async (id: string, isOpen: boolean) => { + // Only fetch when opening a node + if (!isOpen) return; + + const parts = id.split(':'); + const [identifier, databaseId, schema, table] = parts; + const parsedDbId = Number(databaseId); + + if (identifier === 'schema') { + const schemaKey = `${parsedDbId}:${schema}`; + if (!tableData?.[schemaKey]) { + // Set loading state + dispatch({ type: 'SET_LOADING_NODE', nodeId: id, loading: true }); + + // Fetch tables asynchronously + fetchLazyTables( + { + dbId: parsedDbId, + catalog, + schema, + forceRefresh: false, + }, + true, + ) + .then(({ data }) => { + if (data) { + dispatch({ type: 'SET_TABLE_DATA', key: schemaKey, data }); + } + }) + .finally(() => { + dispatch({ + type: 'SET_LOADING_NODE', + nodeId: id, + loading: false, + }); + }); Review Comment: **Suggestion:** Missing await on asynchronous table fetch: the code triggers `fetchLazyTables` and attaches `.then/.finally` but does not await it, so `handleToggle` resolves immediately while the fetch continues in background; await the fetch and use try/finally to ensure loading state is cleared reliably. [race condition] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ⚠️ UI components may proceed before tables finish loading. - ❌ Loading indicator lifecycle may be inconsistent. - ⚠️ Tree expansion consumer logic may observe stale state. ``` </details> ```suggestion try { const result = await fetchLazyTables( { dbId: parsedDbId, catalog, schema, forceRefresh: false, }, true, ); const { data } = result; if (data) { dispatch({ type: 'SET_TABLE_DATA', key: schemaKey, data }); } } finally { dispatch({ type: 'SET_LOADING_NODE', nodeId: id, loading: false, }); } ``` <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. The hook exports `handleToggle` (see return object at lines 315-322 in useTreeData.ts). A consumer (tree component) calls `await handleToggle(id, true)` to expand a schema node and wait for tables to finish loading. 2. `handleToggle` is an async function (defined at line 131) that, for the `schema` case, sets loading state via dispatch at line 144, then calls `fetchLazyTables(...)` at lines 147-155 but does NOT await it — it attaches `.then/.finally` handlers instead (lines 156-167). 3. Because there's no `await` on `fetchLazyTables`, the `handleToggle` async function reaches its end and resolves its returned Promise immediately (no in-function await), so any caller awaiting `handleToggle` will not wait for the table fetch to finish. The loading state is cleared later in the `.finally` callback (lines 161-167), after the caller's await has already resolved. 4. Observable result: consumer code that awaited `handleToggle` may proceed assuming tables are loaded, while in reality the fetch is still in flight and the loading indicator may still be active or cleared later. This follows directly from the call pattern and the return behavior of the async function (see handleToggle definition at lines 131-135 and fetch call at 147-155). ``` </details> <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset-frontend/src/SqlLab/components/TableExploreTree/useTreeData.ts **Line:** 145:167 **Comment:** *Race Condition: Missing await on asynchronous table fetch: the code triggers `fetchLazyTables` and attaches `.then/.finally` but does not await it, so `handleToggle` resolves immediately while the fetch continues in background; await the fetch and use try/finally to ensure loading state is cleared reliably. Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. ``` </details> -- 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]
