codeant-ai-for-open-source[bot] commented on code in PR #36772:
URL: https://github.com/apache/superset/pull/36772#discussion_r2636792444
##########
superset-frontend/plugins/plugin-chart-ag-grid-table/src/AgGridTable/index.tsx:
##########
@@ -53,7 +53,8 @@ import { SearchOutlined } from '@ant-design/icons';
import { debounce, isEqual } from 'lodash';
import Pagination from './components/Pagination';
import SearchSelectDropdown from './components/SearchSelectDropdown';
-import { SearchOption, SortByItem } from '../types';
+import { SearchOption, SortByItem, TableChartFormData } from '../types';
Review Comment:
**Suggestion:** The three identifiers `SearchOption`, `SortByItem`, and
`TableChartFormData` are types; importing them as runtime values may cause
bundling/runtime problems if they are exported only as types. Change this to a
type-only import to ensure the import is erased during compilation. [type error]
**Severity Level:** Minor ⚠️
```suggestion
import type { SearchOption, SortByItem, TableChartFormData } from '../types';
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
Those identifiers are used only as TypeScript types in this file (props and
related types). Converting to `import type { ... }` removes unnecessary runtime
imports and is the correct, idiomatic change.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/plugins/plugin-chart-ag-grid-table/src/AgGridTable/index.tsx
**Line:** 56:56
**Comment:**
*Type Error: The three identifiers `SearchOption`, `SortByItem`, and
`TableChartFormData` are types; importing them as runtime values may cause
bundling/runtime problems if they are exported only as types. Change this to a
type-only import to ensure the import is erased during compilation.
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/plugins/plugin-chart-ag-grid-table/src/AgGridTable/index.tsx:
##########
@@ -53,7 +53,8 @@ import { SearchOutlined } from '@ant-design/icons';
import { debounce, isEqual } from 'lodash';
import Pagination from './components/Pagination';
import SearchSelectDropdown from './components/SearchSelectDropdown';
-import { SearchOption, SortByItem } from '../types';
+import { SearchOption, SortByItem, TableChartFormData } from '../types';
+import { HandlerFunction } from '@superset-ui/core';
Review Comment:
**Suggestion:** Importing `HandlerFunction` as a value import can cause
runtime bundling/import issues if `HandlerFunction` is exported only as a
TypeScript type (not a runtime value). Use a type-only import to ensure the
import is erased at compile time and avoid potential runtime errors. [type
error]
**Severity Level:** Minor ⚠️
```suggestion
import type { HandlerFunction } from '@superset-ui/core';
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
This is a valid, useful change — HandlerFunction is only used as a prop type
in this file, so using `import type` avoids emitting a runtime import and
prevents potential bundling/runtime errors or warnings with type-only exports.
It's safe and low-risk.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/plugins/plugin-chart-ag-grid-table/src/AgGridTable/index.tsx
**Line:** 57:57
**Comment:**
*Type Error: Importing `HandlerFunction` as a value import can cause
runtime bundling/import issues if `HandlerFunction` is exported only as a
TypeScript type (not a runtime value). Use a type-only import to ensure the
import is erased at compile time and avoid potential runtime errors.
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/explore/components/DataTablesPane/components/QueryPane.tsx:
##########
@@ -0,0 +1,206 @@
+/**
+ * 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 { useEffect, useState, useCallback } from 'react';
+import { ensureIsArray, t, getClientErrorObject } from '@superset-ui/core';
+import { css, styled, useTheme } from '@apache-superset/core/ui';
+import {
+ Loading,
+ EmptyState,
+ Icons,
+ Button,
+ Space,
+} from '@superset-ui/core/components';
+import { CopyToClipboard } from 'src/components';
+import { getChartDataRequest } from 'src/components/Chart/chartAction';
+import CodeSyntaxHighlighter, {
+ SupportedLanguage,
+ preloadLanguages,
+} from '@superset-ui/core/components/CodeSyntaxHighlighter';
+import { QueryPaneProps } from '../types';
+
+const QueryPaneContainer = styled.div`
+ ${({ theme }) => css`
+ display: flex;
+ flex-direction: column;
+ height: 100%;
+ padding: ${theme.sizeUnit * 4}px;
+ overflow: auto;
+ `}
+`;
+
+const QueryHeader = styled.div`
+ ${({ theme }) => css`
+ display: flex;
+ justify-content: space-between;
+ align-items: center;
+ margin-bottom: ${theme.sizeUnit * 3}px;
+ flex-shrink: 0;
+ `}
+`;
+
+const QueryTitle = styled.div`
+ ${({ theme }) => css`
+ font-weight: ${theme.fontWeightBold};
+ color: ${theme.colorTextPrimary};
+ `}
+`;
+
+const QueryContent = styled.div`
+ ${({ theme }) => css`
+ flex: 1;
+ overflow: auto;
+ border: 1px solid ${theme.colorBorderSecondary};
+ border-radius: ${theme.borderRadius}px;
+ background: ${theme.colorBgLayout};
+ `}
+`;
+
+const Error = styled.pre`
+ ${({ theme }) => css`
+ margin-top: ${theme.sizeUnit * 4}px;
+ color: ${theme.colorError};
+ `}
+`;
+
+const StyledSyntaxHighlighter = styled(CodeSyntaxHighlighter)`
+ ${({ theme }) => css`
+ margin: 0 !important;
+ padding: ${theme.sizeUnit * 3}px !important;
+ height: 100%;
+ overflow: auto;
+
+ code {
+ font-size: ${theme.fontSizeSM}px;
+ }
+ `}
+`;
+
+interface QueryResult {
+ query?: string;
+ language: SupportedLanguage;
+ error?: string;
+}
+
+export const QueryPane = ({
+ isRequest,
+ queryFormData,
+ isVisible,
+}: QueryPaneProps) => {
+ const theme = useTheme();
+ const [results, setResults] = useState<QueryResult[]>([]);
+ const [isLoading, setIsLoading] = useState(false);
+ const [error, setError] = useState<string | null>(null);
+
+ // Preload SQL language for syntax highlighting
+ useEffect(() => {
+ preloadLanguages(['sql']);
+ }, []);
+
+ useEffect(() => {
+ if (isRequest && isVisible && queryFormData) {
+ setIsLoading(true);
+ setError(null);
+
+ getChartDataRequest({
+ formData: queryFormData,
+ resultFormat: 'json',
+ resultType: 'query',
+ })
+ .then(({ json }) => {
+ setResults(ensureIsArray(json.result));
+ setError(null);
+ })
+ .catch(response => {
+ getClientErrorObject(response).then(({ error, message }) => {
+ setError(
+ error ||
+ message ||
+ response.statusText ||
+ t('An error occurred while fetching the query'),
+ );
+ });
+ })
+ .finally(() => {
+ setIsLoading(false);
+ });
+ }
Review Comment:
**Suggestion:** State update on unmounted component: the async fetch call
can resolve after the component has unmounted and then call state setters
(`setResults`, `setError`, `setIsLoading`), causing React warnings or memory
leaks; guard state updates with an mounted flag and clean it up in the effect's
return callback. [memory leak]
**Severity Level:** Minor ⚠️
```suggestion
let isMounted = true;
if (isRequest && isVisible && queryFormData) {
setIsLoading(true);
setError(null);
getChartDataRequest({
formData: queryFormData,
resultFormat: 'json',
resultType: 'query',
})
.then(({ json }) => {
if (!isMounted) return;
setResults(ensureIsArray(json.result));
setError(null);
})
.catch(response => {
// getClientErrorObject returns a promise; guard updates after it
resolves
getClientErrorObject(response).then(({ error, message }) => {
if (!isMounted) return;
setError(
error ||
message ||
response.statusText ||
t('An error occurred while fetching the query'),
);
});
})
.finally(() => {
if (isMounted) {
setIsLoading(false);
}
});
}
return () => {
isMounted = false;
};
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
Valid suggestion. The effect performs async work and can call state setters
after unmount. Introducing an isMounted/canceled flag (and guarding the promise
resolution) prevents React warnings and pointless state updates. The proposed
change is straightforward and safe.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/src/explore/components/DataTablesPane/components/QueryPane.tsx
**Line:** 116:142
**Comment:**
*Memory Leak: State update on unmounted component: the async fetch call
can resolve after the component has unmounted and then call state setters
(`setResults`, `setError`, `setIsLoading`), causing React warnings or memory
leaks; guard state updates with an mounted flag and clean it up in the effect's
return callback.
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/explore/components/DataTablesPane/components/QueryPane.tsx:
##########
@@ -0,0 +1,206 @@
+/**
+ * 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 { useEffect, useState, useCallback } from 'react';
+import { ensureIsArray, t, getClientErrorObject } from '@superset-ui/core';
+import { css, styled, useTheme } from '@apache-superset/core/ui';
+import {
+ Loading,
+ EmptyState,
+ Icons,
+ Button,
+ Space,
+} from '@superset-ui/core/components';
+import { CopyToClipboard } from 'src/components';
+import { getChartDataRequest } from 'src/components/Chart/chartAction';
+import CodeSyntaxHighlighter, {
+ SupportedLanguage,
+ preloadLanguages,
+} from '@superset-ui/core/components/CodeSyntaxHighlighter';
+import { QueryPaneProps } from '../types';
+
+const QueryPaneContainer = styled.div`
+ ${({ theme }) => css`
+ display: flex;
+ flex-direction: column;
+ height: 100%;
+ padding: ${theme.sizeUnit * 4}px;
+ overflow: auto;
+ `}
+`;
+
+const QueryHeader = styled.div`
+ ${({ theme }) => css`
+ display: flex;
+ justify-content: space-between;
+ align-items: center;
+ margin-bottom: ${theme.sizeUnit * 3}px;
+ flex-shrink: 0;
+ `}
+`;
+
+const QueryTitle = styled.div`
+ ${({ theme }) => css`
+ font-weight: ${theme.fontWeightBold};
+ color: ${theme.colorTextPrimary};
+ `}
+`;
+
+const QueryContent = styled.div`
+ ${({ theme }) => css`
+ flex: 1;
+ overflow: auto;
+ border: 1px solid ${theme.colorBorderSecondary};
+ border-radius: ${theme.borderRadius}px;
+ background: ${theme.colorBgLayout};
+ `}
+`;
+
+const Error = styled.pre`
+ ${({ theme }) => css`
+ margin-top: ${theme.sizeUnit * 4}px;
+ color: ${theme.colorError};
+ `}
+`;
+
+const StyledSyntaxHighlighter = styled(CodeSyntaxHighlighter)`
+ ${({ theme }) => css`
+ margin: 0 !important;
+ padding: ${theme.sizeUnit * 3}px !important;
+ height: 100%;
+ overflow: auto;
+
+ code {
+ font-size: ${theme.fontSizeSM}px;
+ }
+ `}
+`;
+
+interface QueryResult {
+ query?: string;
+ language: SupportedLanguage;
+ error?: string;
+}
+
+export const QueryPane = ({
+ isRequest,
+ queryFormData,
+ isVisible,
+}: QueryPaneProps) => {
+ const theme = useTheme();
+ const [results, setResults] = useState<QueryResult[]>([]);
+ const [isLoading, setIsLoading] = useState(false);
+ const [error, setError] = useState<string | null>(null);
+
+ // Preload SQL language for syntax highlighting
+ useEffect(() => {
+ preloadLanguages(['sql']);
+ }, []);
+
+ useEffect(() => {
+ if (isRequest && isVisible && queryFormData) {
+ setIsLoading(true);
+ setError(null);
+
+ getChartDataRequest({
+ formData: queryFormData,
+ resultFormat: 'json',
+ resultType: 'query',
+ })
+ .then(({ json }) => {
+ setResults(ensureIsArray(json.result));
+ setError(null);
+ })
+ .catch(response => {
+ getClientErrorObject(response).then(({ error, message }) => {
+ setError(
+ error ||
+ message ||
+ response.statusText ||
+ t('An error occurred while fetching the query'),
+ );
+ });
+ })
+ .finally(() => {
+ setIsLoading(false);
+ });
+ }
+ }, [isRequest, isVisible, JSON.stringify(queryFormData)]);
+
+ const getCurrentQuery = useCallback(() => {
+ if (results.length === 0) return '';
+ return results.map(r => r.query || '').join('\n\n');
+ }, [results]);
+
+ if (isLoading) {
+ return <Loading />;
+ }
+
+ if (error) {
+ return (
+ <QueryPaneContainer>
+ <Error>{error}</Error>
+ </QueryPaneContainer>
+ );
+ }
+
+ if (results.length === 0 || !results[0].query) {
Review Comment:
**Suggestion:** Logic bug: the component treats the results as "empty" when
the first result has no `query`, even if later results contain queries; this
will incorrectly render the EmptyState. Replace the check to verify that no
result contains a query instead of only checking the first item. [logic error]
**Severity Level:** Minor ⚠️
```suggestion
if (results.length === 0 || results.every(r => !r.query)) {
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
This is a real logic bug: the current code treats the whole result set as
empty if the first item lacks a query even when later results contain SQL.
Replacing the check with results.every(r => !r.query) fixes that and aligns
with getCurrentQuery which aggregates all results.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/src/explore/components/DataTablesPane/components/QueryPane.tsx
**Line:** 162:162
**Comment:**
*Logic Error: Logic bug: the component treats the results as "empty"
when the first result has no `query`, even if later results contain queries;
this will incorrectly render the EmptyState. Replace the check to verify that
no result contains a query instead of only checking the first item.
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/plugins/plugin-chart-ag-grid-table/src/transformProps.ts:
##########
@@ -469,7 +469,7 @@ const transformProps = (
queriesData = [],
ownState: serverPaginationData,
filterState,
- hooks: { setDataMask = () => {}, onChartStateChange },
+ hooks: { setDataMask = () => {}, onChartStateChange, setControlValue },
Review Comment:
**Suggestion:** Destructuring `hooks` directly from `chartProps` without a
default for the parent object will throw if `chartProps.hooks` is undefined
(TypeError: cannot destructure property of 'undefined'). Provide a default
empty object for `hooks` in the destructuring so the nested defaults are
applied safely. [null pointer]
**Severity Level:** Minor ⚠️
```suggestion
hooks: { setDataMask = () => {}, onChartStateChange, setControlValue } =
{},
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
Accurate. If chartProps.hooks is undefined, destructuring its properties
will throw at runtime. Defaulting the parent object (hooks = {}) is a small,
defensive change that prevents a TypeError and is safe even if the type
normally guarantees hooks exists.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/plugins/plugin-chart-ag-grid-table/src/transformProps.ts
**Line:** 472:472
**Comment:**
*Null Pointer: Destructuring `hooks` directly from `chartProps` without
a default for the parent object will throw if `chartProps.hooks` is undefined
(TypeError: cannot destructure property of 'undefined'). Provide a default
empty object for `hooks` in the destructuring so the nested defaults are
applied safely.
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/plugins/plugin-chart-ag-grid-table/src/types.ts:
##########
@@ -43,6 +43,7 @@ import {
IHeaderParams,
CustomCellRendererProps,
} from '@superset-ui/core/components/ThemedAgGridReact';
+import { HandlerFunction } from '@superset-ui/core';
Review Comment:
**Suggestion:** Non-type import of `HandlerFunction` can emit a runtime
import for a symbol that may only exist as a TypeScript type and not as a JS
value; that can produce a runtime "undefined" import or bundler/runtime error.
Import type-only declarations using `import type` to ensure no runtime import
is emitted. [type error]
**Severity Level:** Minor ⚠️
```suggestion
import type { HandlerFunction } from '@superset-ui/core';
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
HandlerFunction is only used in type positions in this file (interfaces), so
switching to `import type` avoids emitting a runtime import for a symbol that
is only a TS type. This reduces bundle ambiguity and is the correct modern TS
usage.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/plugins/plugin-chart-ag-grid-table/src/types.ts
**Line:** 46:46
**Comment:**
*Type Error: Non-type import of `HandlerFunction` can emit a runtime
import for a symbol that may only exist as a TypeScript type and not as a JS
value; that can produce a runtime "undefined" import or bundler/runtime error.
Import type-only declarations using `import type` to ensure no runtime import
is emitted.
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/plugins/plugin-chart-ag-grid-table/src/transformProps.ts:
##########
@@ -739,6 +739,7 @@ const transformProps = (
formData,
chartState: serverPaginationData?.chartState,
onChartStateChange,
+ setControlValue,
Review Comment:
**Suggestion:** The returned props now include `setControlValue` (added to
the returned object) but it may be undefined; returning an undefined value that
callers will invoke can cause runtime errors. Ensure the returned property is
always a function by coalescing to a noop when `setControlValue` is falsy.
[possible bug]
**Severity Level:** Critical 🚨
```suggestion
setControlValue: setControlValue ?? (() => {}),
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
Valid: ensuring the returned prop is always a function (coalesce to a noop)
prevents consumers from having to null-check before calling it. This duplicates
the safety of providing a default during destructuring, but is a safe fallback
if you can't change the destructuring.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/plugins/plugin-chart-ag-grid-table/src/transformProps.ts
**Line:** 742:742
**Comment:**
*Possible Bug: The returned props now include `setControlValue` (added
to the returned object) but it may be undefined; returning an undefined value
that callers will invoke can cause runtime errors. Ensure the returned property
is always a function by coalescing to a noop when `setControlValue` is falsy.
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/explore/components/DataTablesPane/components/QueryPane.tsx:
##########
@@ -0,0 +1,206 @@
+/**
+ * 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 { useEffect, useState, useCallback } from 'react';
+import { ensureIsArray, t, getClientErrorObject } from '@superset-ui/core';
+import { css, styled, useTheme } from '@apache-superset/core/ui';
+import {
+ Loading,
+ EmptyState,
+ Icons,
+ Button,
+ Space,
+} from '@superset-ui/core/components';
+import { CopyToClipboard } from 'src/components';
+import { getChartDataRequest } from 'src/components/Chart/chartAction';
+import CodeSyntaxHighlighter, {
+ SupportedLanguage,
+ preloadLanguages,
+} from '@superset-ui/core/components/CodeSyntaxHighlighter';
+import { QueryPaneProps } from '../types';
+
+const QueryPaneContainer = styled.div`
+ ${({ theme }) => css`
+ display: flex;
+ flex-direction: column;
+ height: 100%;
+ padding: ${theme.sizeUnit * 4}px;
+ overflow: auto;
+ `}
+`;
+
+const QueryHeader = styled.div`
+ ${({ theme }) => css`
+ display: flex;
+ justify-content: space-between;
+ align-items: center;
+ margin-bottom: ${theme.sizeUnit * 3}px;
+ flex-shrink: 0;
+ `}
+`;
+
+const QueryTitle = styled.div`
+ ${({ theme }) => css`
+ font-weight: ${theme.fontWeightBold};
+ color: ${theme.colorTextPrimary};
+ `}
+`;
+
+const QueryContent = styled.div`
+ ${({ theme }) => css`
+ flex: 1;
+ overflow: auto;
+ border: 1px solid ${theme.colorBorderSecondary};
+ border-radius: ${theme.borderRadius}px;
+ background: ${theme.colorBgLayout};
+ `}
+`;
+
+const Error = styled.pre`
+ ${({ theme }) => css`
+ margin-top: ${theme.sizeUnit * 4}px;
+ color: ${theme.colorError};
+ `}
+`;
+
+const StyledSyntaxHighlighter = styled(CodeSyntaxHighlighter)`
+ ${({ theme }) => css`
+ margin: 0 !important;
+ padding: ${theme.sizeUnit * 3}px !important;
+ height: 100%;
+ overflow: auto;
+
+ code {
+ font-size: ${theme.fontSizeSM}px;
+ }
+ `}
+`;
+
+interface QueryResult {
+ query?: string;
+ language: SupportedLanguage;
Review Comment:
**Suggestion:** Type/contract mismatch: `QueryResult.language` is typed as
required but code defensively uses `result.language || 'sql'`; either mark
`language` optional in the `QueryResult` interface or ensure the fetched
results always set a valid `SupportedLanguage`. Make `language` optional to
reflect runtime usage. [type error]
**Severity Level:** Minor ⚠️
```suggestion
language?: SupportedLanguage;
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
The render uses result.language || 'sql', implying language may be undefined
at runtime. Making language optional in the local interface aligns types with
actual usage and avoids false positives from TypeScript. It's a small, correct
relaxation of the contract.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/src/explore/components/DataTablesPane/components/QueryPane.tsx
**Line:** 96:96
**Comment:**
*Type Error: Type/contract mismatch: `QueryResult.language` is typed as
required but code defensively uses `result.language || 'sql'`; either mark
`language` optional in the `QueryResult` interface or ensure the fetched
results always set a valid `SupportedLanguage`. Make `language` optional to
reflect runtime usage.
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/plugins/plugin-chart-ag-grid-table/src/styles/index.tsx:
##########
@@ -139,6 +139,28 @@ export const MenuContainer = styled.div`
background-color: ${theme.colorBorderSecondary};
margin: ${theme.sizeUnit}px 0;
}
+
+ .menu-submenu {
+ position: relative;
+
+ .submenu {
+ display: none;
+ position: absolute;
+ left: 100%;
+ top: 0;
+ min-width: ${theme.sizeUnit * 35}px;
+ background: var(--ag-menu-background-color, ${theme.colorBgBase});
+ border: var(--ag-menu-border, 1px solid ${theme.colorBorderSecondary});
+ box-shadow: var(--ag-menu-shadow, ${theme.boxShadow});
+ border-radius: ${theme.borderRadius}px;
+ padding: ${theme.sizeUnit}px 0;
+ z-index: 100;
Review Comment:
**Suggestion:** Hardcoded stacking order: the submenu uses a fixed `z-index:
100` which can collide with other components; make it configurable via a CSS
variable with a sensible fallback so it can be adjusted where the menu is used.
[possible bug]
**Severity Level:** Critical 🚨
```suggestion
z-index: var(--ag-submenu-z-index, 100);
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
Making the z-index configurable via a CSS variable (with a sensible
fallback) prevents stacking collisions
with other components and is a low-risk improvement. The proposed improved
code does exactly that.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/plugins/plugin-chart-ag-grid-table/src/styles/index.tsx
**Line:** 157:157
**Comment:**
*Possible Bug: Hardcoded stacking order: the submenu uses a fixed
`z-index: 100` which can collide with other components; make it configurable
via a CSS variable with a sensible fallback so it can be adjusted where the
menu is used.
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/plugins/plugin-chart-table/src/ColumnHeaderContextMenu.tsx:
##########
@@ -0,0 +1,236 @@
+/**
+ * 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, useMemo } from 'react';
+import { t, ensureIsArray, HandlerFunction } from '@superset-ui/core';
+import { css, useTheme } from '@apache-superset/core/ui';
+import { Dropdown, Menu } from '@superset-ui/core/components';
+import { GenericDataType } from '@apache-superset/core/api/core';
+import {
+ GroupOutlined,
+ FilterOutlined,
+ CalculatorOutlined,
+ EyeInvisibleOutlined,
+ SortAscendingOutlined,
+ SortDescendingOutlined,
+} from '@ant-design/icons';
+import { DataColumnMeta, TableChartFormData } from './types';
+
+export interface ColumnHeaderContextMenuProps {
+ column: DataColumnMeta;
+ formData?: TableChartFormData;
+ setControlValue?: HandlerFunction;
+ children: React.ReactNode;
+ disabled?: boolean;
+}
+
+type MenuAction =
+ | 'groupby'
+ | 'sum'
+ | 'avg'
+ | 'count'
+ | 'count_distinct'
+ | 'min'
+ | 'max'
+ | 'filter'
+ | 'hide'
+ | 'sort_asc'
+ | 'sort_desc';
+
+export default function ColumnHeaderContextMenu({
+ column,
+ formData,
+ setControlValue,
+ children,
+ disabled = false,
+}: ColumnHeaderContextMenuProps) {
+ const theme = useTheme();
+
+ const isNumericColumn =
+ column.dataType === GenericDataType.Numeric || column.isNumeric;
+ const isMetric = column.isMetric || column.isPercentMetric;
+ const columnName = column.key;
+
+ const handleMenuClick = useCallback(
+ (action: MenuAction) => {
+ if (!setControlValue || !formData) return;
+
+ const currentGroupby = ensureIsArray(formData.groupby);
+ const currentMetrics = ensureIsArray(formData.metrics);
+
+ switch (action) {
+ case 'groupby':
+ // Add column to groupby if not already present
+ if (!currentGroupby.includes(columnName)) {
+ setControlValue('groupby', [...currentGroupby, columnName]);
+ }
+ break;
+
+ case 'sum':
+ case 'avg':
+ case 'count':
+ case 'min':
+ case 'max': {
+ // Create an aggregate metric
+ const aggregate = action.toUpperCase();
+ const metricLabel = `${aggregate}(${columnName})`;
+ const newMetric = {
+ aggregate,
+ column: { column_name: columnName },
+ expressionType: 'SIMPLE',
+ label: metricLabel,
+ };
+ setControlValue('metrics', [...currentMetrics, newMetric]);
+ break;
+ }
+
+ case 'count_distinct': {
+ const metricLabel = `COUNT_DISTINCT(${columnName})`;
+ const newMetric = {
+ aggregate: 'COUNT_DISTINCT',
+ column: { column_name: columnName },
+ expressionType: 'SIMPLE',
+ label: metricLabel,
+ };
+ setControlValue('metrics', [...currentMetrics, newMetric]);
+ break;
+ }
+
+ case 'filter':
+ // Open filter modal - for now just add a placeholder filter
+ // This would ideally open a filter UI
+ break;
+
+ case 'hide': {
+ // Update column_config to hide this column
+ const currentColumnConfig = formData.column_config || {};
+ setControlValue('column_config', {
+ ...currentColumnConfig,
+ [columnName]: {
+ ...currentColumnConfig[columnName],
+ visible: false,
+ },
+ });
+ break;
+ }
+
+ default:
+ break;
+ }
+ },
+ [setControlValue, formData, columnName],
+ );
+
+ const menuItems = useMemo(() => {
+ const items: React.ComponentProps<typeof Menu>['items'] = [];
+
+ // Group By option (only for non-metric columns)
+ if (!isMetric) {
+ items.push({
+ key: 'groupby',
+ icon: <GroupOutlined />,
+ label: t('Group by this column'),
+ onClick: () => handleMenuClick('groupby'),
+ });
+ }
+
+ // Aggregate options (for numeric columns or any column for COUNT)
+ if (!isMetric) {
+ const aggregateItems = [
+ {
+ key: 'count',
+ label: t('COUNT'),
+ onClick: () => handleMenuClick('count'),
+ },
+ {
+ key: 'count_distinct',
+ label: t('COUNT DISTINCT'),
+ onClick: () => handleMenuClick('count_distinct'),
+ },
+ ];
+
+ if (isNumericColumn) {
+ aggregateItems.unshift(
+ {
+ key: 'sum',
+ label: t('SUM'),
+ onClick: () => handleMenuClick('sum'),
+ },
+ {
+ key: 'avg',
+ label: t('AVG'),
+ onClick: () => handleMenuClick('avg'),
+ },
+ {
+ key: 'min',
+ label: t('MIN'),
+ onClick: () => handleMenuClick('min'),
+ },
+ {
+ key: 'max',
+ label: t('MAX'),
+ onClick: () => handleMenuClick('max'),
+ },
+ );
+ }
+
+ items.push({
+ key: 'aggregate',
+ icon: <CalculatorOutlined />,
+ label: t('Add as metric'),
+ children: aggregateItems,
+ });
+ }
+
+ // Divider
+ if (items.length > 0) {
+ items.push({ type: 'divider' });
+ }
+
+ // Hide column
+ items.push({
+ key: 'hide',
+ icon: <EyeInvisibleOutlined />,
+ label: t('Hide column'),
+ onClick: () => handleMenuClick('hide'),
+ });
+
+ return items;
+ }, [isMetric, isNumericColumn, handleMenuClick]);
+
+ // If no setControlValue, just render children without dropdown
+ if (!setControlValue || disabled) {
+ return <>{children}</>;
+ }
+
+ return (
+ <Dropdown
+ menu={{ items: menuItems }}
Review Comment:
**Suggestion:** The Dropdown's menu items include per-item `onClick`
handlers but the `Dropdown`/`Menu` API expects a top-level `onClick` in the
`menu` prop (item-level `onClick` may be ignored depending on Ant Design
version), so clicks won't invoke `handleMenuClick`. Add a top-level `onClick`
that maps the clicked item's `key` to `handleMenuClick`. [logic error]
**Severity Level:** Minor ⚠️
```suggestion
menu={{
items: menuItems,
onClick: ({ key }) => handleMenuClick(key as MenuAction),
}}
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
Ant Design's Dropdown/menu pattern relies on the menu's onClick handler to
receive item clicks in many versions; embedding per-item onClick in the items
array is unreliable across versions. Adding menu.onClick that maps keys to the
existing handleMenuClick ensures clicks are always handled and fixes a real
functional bug in this component.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/plugins/plugin-chart-table/src/ColumnHeaderContextMenu.tsx
**Line:** 223:223
**Comment:**
*Logic Error: The Dropdown's menu items include per-item `onClick`
handlers but the `Dropdown`/`Menu` API expects a top-level `onClick` in the
`menu` prop (item-level `onClick` may be ignored depending on Ant Design
version), so clicks won't invoke `handleMenuClick`. Add a top-level `onClick`
that maps the clicked item's `key` to `handleMenuClick`.
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]
