codeant-ai-for-open-source[bot] commented on code in PR #37298:
URL: https://github.com/apache/superset/pull/37298#discussion_r2710427073
##########
superset-frontend/src/SqlLab/components/AppLayout/index.tsx:
##########
@@ -99,47 +139,59 @@ const AppLayout: React.FC = ({ children }) => {
return (
<StyledContainer>
- <Splitter
- css={css`
- flex: 1;
- `}
- lazy
- onResizeEnd={onSidebarChange}
- onResize={noop}
- >
- <Splitter.Panel
- collapsible={{
- start: true,
- end: true,
- showCollapsibleIcon: true,
- }}
- size={leftWidth}
- min={SQL_EDITOR_LEFTBAR_WIDTH}
+ <StyledSidebarWrapper>
+ <Layout.Sider
+ collapsed
Review Comment:
**Suggestion:** Logic bug: `Layout.Sider` is hard-coded to be collapsed
because the `collapsed` prop is provided without a value (equivalent to true).
This forces the sidebar to always render in its collapsed state and never
expand; toggle/resize logic will not make it expand. Make `collapsed`
controlled by `leftWidth` so the Sider collapses/expands according to state.
[logic error]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ SQL Lab left sidebar cannot expand (AppLayout component).
- ⚠️ Database explorer toggle click has no visible effect.
```
</details>
```suggestion
collapsed={leftWidth <= SQL_EDITOR_LEFTBAR_COLLAPSED_WIDTH}
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open the SQL Lab UI which mounts AppLayout (component defined in
superset-frontend/src/SqlLab/components/AppLayout/index.tsx). The AppLayout
render begins
at the return block where the sidebar is declared (see Layout.Sider at lines
143-146 of
the PR diff).
2. Inspect toggle logic: toggleSidebar useCallback is declared earlier in
the same file
(toggleSidebar at line 96 in the PR hunk) and sets leftWidth to
SQL_EDITOR_LEFTBAR_WIDTH
when expanding (line 98), and to SQL_EDITOR_LEFTBAR_COLLAPSED_WIDTH when
collapsing (line
100).
3. Interact with the collapsed menu: the collapsed Menu is rendered inside
the Sider
(StyledMenu at lines 147-151). Clicking the menu item calls toggleSidebar
(collapsedMenuItems definition at lines 104-115 which uses toggleSidebar).
4. Observe behavior: despite toggleSidebar updating leftWidth, Layout.Sider
is hard-coded
with the boolean prop collapsed (see lines 143-146). Because collapsed is
always true, the
Sider remains visually collapsed and never expands even after toggleSidebar
executes —
reproducing the bug (click collapsed menu item, sidebar does not expand).
```
</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/AppLayout/index.tsx
**Line:** 144:144
**Comment:**
*Logic Error: Logic bug: `Layout.Sider` is hard-coded to be collapsed
because the `collapsed` prop is provided without a value (equivalent to true).
This forces the sidebar to always render in its collapsed state and never
expand; toggle/resize logic will not make it expand. Make `collapsed`
controlled by `leftWidth` so the Sider collapses/expands according to state.
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/SqlEditorTopBar/index.tsx:
##########
@@ -16,12 +16,14 @@
* specific language governing permissions and limitations
* under the License.
*/
-import { Divider, Flex } from '@superset-ui/core/components';
-import { styled } from '@apache-superset/core/ui';
+import { Divider, EmptyState, Flex } from '@superset-ui/core/components';
+import { css, styled } from '@apache-superset/core/ui';
Review Comment:
**Suggestion:** The `css` template tag is imported from
`@apache-superset/core/ui`, but that package may not export a `css` template
function (the project normally sources the Emotion `css` from
`@emotion/react`). If `css` is undefined at runtime the `css\`...\`` call will
throw. Import `css` from `@emotion/react` (and keep `styled` from the current
UI package) to ensure the template tag exists. [possible bug]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ SqlEditorTopBar fails to render in SQLLab.
- ❌ DatabaseSelector modal inaccessible from top bar.
- ⚠️ Primary/secondary menu actions hidden.
```
</details>
```suggestion
import { styled } from '@apache-superset/core/ui';
import { css } from '@emotion/react';
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Build and run the frontend with the PR changes so the SQL editor loads.
The component
file to exercise is
superset-frontend/src/SqlLab/components/SqlEditorTopBar/index.tsx.
2. Navigate to SQLLab so React mounts SqlEditorTopBar (the default export
from the file
above). The component renders the JSX block that uses the `css` template tag
at the Flex
prop (see the same file where css is used at the block starting around the
`css\`` usage).
3. At render time the expression css`min-width: 0; overflow: hidden;` (in
this file) is
evaluated; if `css` imported from '@apache-superset/core/ui' is undefined,
React will
throw TypeError: css is not a function when evaluating the template tag.
4. Observed failure: SQL editor top bar fails to render and a runtime error
appears in the
browser console referencing
superset-frontend/src/SqlLab/components/SqlEditorTopBar/index.tsx (the
`css\`` usage
site).
Explanation: the project package `@apache-superset/core/ui` (see
packages/superset-core/src/ui/theme exports) does not export `css` in
this codebase;
keeping `css` imported from there is likely incorrect. Replacing the
import with `css`
from '@emotion/react' matches how template tags are provided in other
code.
```
</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/SqlEditorTopBar/index.tsx
**Line:** 20:20
**Comment:**
*Possible Bug: The `css` template tag is imported from
`@apache-superset/core/ui`, but that package may not export a `css` template
function (the project normally sources the Emotion `css` from
`@emotion/react`). If `css` is undefined at runtime the `css\`...\`` call will
throw. Import `css` from `@emotion/react` (and keep `styled` from the current
UI package) to ensure the template tag exists.
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/packages/superset-ui-core/src/components/Segmented/types.ts:
##########
@@ -0,0 +1,21 @@
+/**
+ * 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 type { SegmentedProps } from 'antd/es/segmented';
+
+export { SegmentedProps };
Review Comment:
**Suggestion:** Type-only import is being re-exported as a value: `import
type { SegmentedProps }` followed by `export { SegmentedProps }` attempts to
export a runtime binding that doesn't exist and will produce a TypeScript
compile error. Use a type-only export (`export type { ... }`) to re-export the
type. [type error]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ superset-frontend TypeScript build failure.
- ❌ CI frontend build job fails blocking deployments.
- ⚠️ Local developer typecheck/watch sessions error.
```
</details>
```suggestion
export type { SegmentedProps };
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. From the repository root run the frontend typecheck/build (e.g., `yarn
build` or the
monorepo TypeScript build). The TypeScript compiler will read the file at
`superset-frontend/packages/superset-ui-core/src/components/Segmented/types.ts:19-21`.
2. TypeScript resolves the `import type { SegmentedProps }` at line 19 and
then encounters
`export { SegmentedProps };` at line 21 which attempts to export a runtime
binding that
does not exist.
3. The compiler emits an error during the build/typecheck step referencing
`.../Segmented/types.ts:21` and the build fails (CI/frontend build job fails
at the
TypeScript phase).
4. Confirm reproduction by opening
`superset-frontend/packages/superset-ui-core/src/components/Segmented/types.ts`
and
running `tsc --noEmit` locally; the error points to line 21 showing the
invalid re-export
of a type-only import.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/packages/superset-ui-core/src/components/Segmented/types.ts
**Line:** 21:21
**Comment:**
*Type Error: Type-only import is being re-exported as a value: `import
type { SegmentedProps }` followed by `export { SegmentedProps }` attempts to
export a runtime binding that doesn't exist and will produce a TypeScript
compile error. Use a type-only export (`export type { ... }`) to re-export the
type.
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,685 @@
+/**
+ * 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,
+ useCallback,
+ useState,
+ useRef,
+ type ChangeEvent,
+} from 'react';
+import { useSelector, useDispatch, shallowEqual } from 'react-redux';
+import { css, 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, NodeRendererProps, NodeApi } from 'react-arborist';
+import {
+ Icons,
+ Skeleton,
+ Input,
+ Tooltip,
+ Empty,
+ Typography,
+ Button,
+} from '@superset-ui/core/components';
+import RefreshLabel from '@superset-ui/core/components/RefreshLabel';
+import type { SqlLabRootState } from 'src/SqlLab/types';
+import ColumnElement, {
+ type ColumnKeyTypeType,
+} from 'src/SqlLab/components/ColumnElement';
+import {
+ Table,
+ TableMetaData,
+ useSchemas,
+ useLazyTablesQuery,
+ useLazyTableMetadataQuery,
+ useLazyTableExtendedMetadataQuery,
+} from 'src/hooks/apiResources';
+import useQueryEditor from 'src/SqlLab/hooks/useQueryEditor';
+import { addTable } from 'src/SqlLab/actions/sqlLab';
+import IconButton from 'src/dashboard/components/IconButton';
+import ButtonCell from
'@superset-ui/core/components/Table/cell-renderers/ButtonCell';
+
+type Props = {
+ queryEditorId: string;
+};
+
+interface TreeNodeData {
+ id: string;
+ name: string;
+ type: 'schema' | 'table' | 'column' | 'empty';
+ tableType?: string;
+ columnData?: {
+ name: string;
+ keys?: { type: ColumnKeyTypeType }[];
+ type: string;
+ };
+ children?: TreeNodeData[];
+ disableCheckbox?: boolean;
+}
+
+const EMPTY_NODE_ID_PREFIX = 'empty:';
+
+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 getOpacity = (disableCheckbox: boolean | undefined) => ({
+ opacity: 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}
+ </>
+ );
+};
+
+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 } = queryEditor;
+ const pinnedTables = useMemo(
+ () =>
+ Object.fromEntries(
+ tables.map(({ queryEditorId, dbId, schema, name, persistData }) => [
+ queryEditor.id === queryEditorId ? `${dbId}:${schema}:${name}` : '',
+ persistData,
+ ]),
+ ),
+ [tables, queryEditor.id],
+ );
+ const {
+ currentData: schemaData,
+ isFetching,
+ refetch,
+ } = useSchemas({ dbId, catalog: catalog || undefined });
+ const [tableData, setTableData] =
+ useState<Record<string, { options: Table[] }>>();
Review Comment:
**Suggestion:** Runtime error: `tableData` is initialized as undefined, but
later updates spread the previous state (e.g. `{ ...origin, [schemaKey]: data
}`), which will throw when `origin` is undefined; initialize `tableData` to an
empty object to avoid spreading `undefined`. [possible bug]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ Expanding schema crashes tree UI.
- ⚠️ Table children fail to populate for schemas.
```
</details>
```suggestion
useState<Record<string, { options: Table[] }>>({});
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Render the TableExploreTree component (file:
superset-frontend/src/SqlLab/components/TableExploreTree/index.tsx).
tableData is
initialized at lines 194-195 as undefined.
2. Expand a schema node in the UI (user clicks the schema row). The Tree
onToggle handler
at lines ~655-673 calls handleToggle for that node id.
3. handleToggle (file lines 328-431) detects identifier === 'schema', calls
fetchLazyTables(...). In the .then() handler (lines ~355-363) it calls
setTableData(origin
=> ({ ...origin, [schemaKey]: data })).
4. Because the initial state origin is undefined, spreading origin causes a
runtime
TypeError (spreading non-object) at the .then() handler, observed as an
exception when
expanding any schema for the first time.
5. Initializing tableData to an empty object (improved code) prevents
spreading undefined
and resolves the crash when loading tables for a schema.
```
</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:** 195:195
**Comment:**
*Possible Bug: Runtime error: `tableData` is initialized as undefined,
but later updates spread the previous state (e.g. `{ ...origin, [schemaKey]:
data }`), which will throw when `origin` is undefined; initialize `tableData`
to an empty object to avoid spreading `undefined`.
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]
