codeant-ai-for-open-source[bot] commented on code in PR #36390:
URL: https://github.com/apache/superset/pull/36390#discussion_r2583772656
##########
superset-frontend/src/dashboard/util/constants.ts:
##########
@@ -31,6 +31,9 @@ export const NEW_MARKDOWN_ID = 'NEW_MARKDOWN_ID';
export const NEW_ROW_ID = 'NEW_ROW_ID';
export const NEW_TAB_ID = 'NEW_TAB_ID';
export const NEW_TABS_ID = 'NEW_TABS_ID';
+export const NEW_BUTTON_ID = 'NEW_BUTTON_ID';
+export const NEW_MODEL3D_ID = 'NEW_MODEL3D_ID';
+export const NEW_ALERTS_ID = 'NEW_ALERTS_ID';
Review Comment:
**Suggestion:** The newly added constants for button, 3D model, and alerts
IDs are not referenced anywhere else in the codebase, which introduces dead
code and can confuse future maintainers; they should be removed until they are
actually needed and wired into the dashboard logic. [code quality]
**Severity Level:** Minor ⚠️
```suggestion
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
A search across the repository confirms these three newly added ID
constants are not referenced anywhere else, so they currently serve no
purpose and just clutter the constants module. Removing them until the
associated features are implemented avoids dead exports that suggest
non‑existent functionality. This is a reasonable, concrete clean‑up and
does not conflict with the PR's changes.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/dashboard/util/constants.ts
**Line:** 34:36
**Comment:**
*Code Quality: The newly added constants for button, 3D model, and
alerts IDs are not referenced anywhere else in the codebase, which introduces
dead code and can confuse future maintainers; they should be removed until they
are actually needed and wired into the dashboard logic.
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/dashboard/components/gridComponents/Alerts/AlertsConfigMenuItem.tsx:
##########
@@ -0,0 +1,286 @@
+/**
+ * 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, useState } from 'react';
+import { t } from '@superset-ui/core';
+import { styled, css } from '@apache-superset/core/ui';
+import { Button as SupersetButton } from '@superset-ui/core/components/Button';
+import { Form } from '@superset-ui/core/components/Form';
+import { Input, Select, Tooltip } from '@superset-ui/core/components';
+import { Switch } from '@superset-ui/core/components/Switch';
+import { Modal } from '@superset-ui/core/components';
+import { Icons } from '@superset-ui/core/components/Icons';
+import type { DashboardAlertsMeta } from './types';
+
+const MenuTrigger = styled.div`
+ ${({ theme }) => css`
+ display: flex;
+ align-items: center;
+ justify-content: center;
+ cursor: pointer;
+ color: ${theme.colorTextDescription};
+
+ &:hover {
+ color: ${theme.colorPrimary};
+ }
+ `}
+`;
+
+const ConfigFormWrapper = styled.div`
+ ${({ theme }) => css`
+ .ant-form-item {
+ margin-bottom: ${theme.sizeUnit * 3}px;
+ }
+
+ .config-form-footer {
+ display: flex;
+ gap: ${theme.sizeUnit * 2}px;
+ justify-content: flex-end;
+ margin-top: ${theme.sizeUnit * 4}px;
+ padding-top: ${theme.sizeUnit * 3}px;
+ border-top: 1px solid ${theme.colorBorder};
+ }
+
+ .form-section-title {
+ font-weight: ${theme.fontWeightBold};
+ margin-bottom: ${theme.sizeUnit * 2}px;
+ color: ${theme.colorText};
+ }
+
+ .form-help-text {
+ font-size: ${theme.fontSizeSm}px;
+ color: ${theme.colorTextDescription};
+ margin-top: ${theme.sizeUnit}px;
+ }
+ `}
+`;
+
+export type AlertsConfig = Pick<
+ DashboardAlertsMeta,
+ | 'mqttTopic'
+ | 'includeGlobalTopic'
+ | 'eventFilter'
+ | 'severityFilter'
+ | 'showVisualIndicator'
+ | 'indicatorColor'
+>;
+
+type FormValues = AlertsConfig;
+
+interface AlertsConfigMenuItemProps {
+ meta: DashboardAlertsMeta;
+ onSave: (updates: AlertsConfig) => void;
+ onVisibilityChange?: (open: boolean) => void;
+}
+
+function getInitialValues(meta: DashboardAlertsMeta): FormValues {
+ return {
+ mqttTopic: meta.mqttTopic ?? '',
+ includeGlobalTopic: meta.includeGlobalTopic ?? true,
+ eventFilter: meta.eventFilter ?? '',
+ severityFilter: meta.severityFilter ?? [],
+ showVisualIndicator: meta.showVisualIndicator ?? true,
+ indicatorColor: meta.indicatorColor ?? '#1890ff',
+ };
+}
+
+const AlertsConfigMenuItem = ({
+ meta,
+ onSave,
+ onVisibilityChange,
+}: AlertsConfigMenuItemProps) => {
+ const [modalVisible, setModalVisible] = useState(false);
+ const [form] = Form.useForm<FormValues>();
+
+ const initialValues = useMemo(() => getInitialValues(meta), [meta]);
+
+ const severityOptions = useMemo(
+ () => [
+ { value: 'info', label: t('Info') },
+ { value: 'success', label: t('Success') },
+ { value: 'warning', label: t('Warning') },
+ { value: 'error', label: t('Error') },
+ ],
+ [],
+ );
+
+ const handleOpenModal = useCallback(() => {
+ form.setFieldsValue(initialValues);
+ setModalVisible(true);
+ onVisibilityChange?.(true);
+ }, [form, initialValues, onVisibilityChange]);
+
+ const handleCloseModal = useCallback(() => {
+ setModalVisible(false);
+ onVisibilityChange?.(false);
+ }, [onVisibilityChange]);
+
+ const handleSubmit = useCallback(() => {
+ form
+ .validateFields()
+ .then(values => {
+ const updates: AlertsConfig = {
+ mqttTopic: values.mqttTopic?.trim() || '',
+ includeGlobalTopic: values.includeGlobalTopic ?? true,
+ eventFilter: values.eventFilter?.trim() || '',
+ severityFilter: values.severityFilter || [],
+ showVisualIndicator: values.showVisualIndicator ?? true,
+ indicatorColor: values.indicatorColor || '#1890ff',
+ };
+ onSave(updates);
+ handleCloseModal();
+ })
+ .catch(error => {
+ console.error('Form validation failed:', error);
+ });
+ }, [form, onSave, handleCloseModal]);
+
+ return (
+ <>
+ <Tooltip title={t('Configure alerts')} placement="top">
+ <MenuTrigger onClick={handleOpenModal}>
+ <Icons.SettingOutlined />
+ </MenuTrigger>
+ </Tooltip>
+
+ <Modal
+ title={t('Configure Alert Listener')}
+ visible={modalVisible}
+ onCancel={handleCloseModal}
+ width={600}
+ footer={null}
+ destroyOnClose
+ >
+ <ConfigFormWrapper>
+ <Form
+ form={form}
+ layout="vertical"
+ initialValues={initialValues}
+ preserve={false}
+ >
+ {/* MQTT Topic Configuration */}
+ <div className="form-section-title">{t('MQTT Topic')}</div>
+
+ <Form.Item
+ name="mqttTopic"
+ label={t('Custom Topic')}
+ extra={t('Enter the MQTT topic to subscribe to (e.g.,
"temperature/alerts", "dashboard/123/events")')}
+ rules={[
+ {
+ validator: (_, value) => {
+ if (!value || value.trim().length === 0) {
+ return Promise.reject(
+ new Error(t('Please enter an MQTT topic')),
+ );
+ }
+ // Basic topic validation
+ if (value.includes('#') && !value.endsWith('#')) {
Review Comment:
**Suggestion:** The MQTT topic validator operates on the raw input string
instead of a trimmed version, so a topic that is valid once trimmed (for
example, with trailing or leading spaces) will be rejected by validation even
though `handleSubmit` later trims and saves it, causing the UI to block valid
inputs and behave inconsistently with the saved value. [logic error]
**Severity Level:** Minor ⚠️
```suggestion
const trimmedValue = (value || '').trim();
if (trimmedValue.length === 0) {
return Promise.reject(
new Error(t('Please enter an MQTT topic')),
);
}
// Basic topic validation
if (
trimmedValue.includes('#') &&
!trimmedValue.endsWith('#')
) {
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
The current validator mixes trimmed and untrimmed values: emptiness is
checked on `value.trim()`, but the wildcard rule uses the raw `value`. That
means an input like `"topic/# "` will be rejected by the validator (because it
doesn't literally end with `'#'`), even though `handleSubmit` later trims and
persists `"topic/#"`, which is valid. That's a real logic/UX bug where
validation blocks a value that the submit logic would normalize and accept. The
suggested change consistently trims once up front and applies both checks to
the trimmed string, aligning validation with the saved value and avoiding
spurious rejections, without introducing any nonsense.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/src/dashboard/components/gridComponents/Alerts/AlertsConfigMenuItem.tsx
**Line:** 186:192
**Comment:**
*Logic Error: The MQTT topic validator operates on the raw input string
instead of a trimmed version, so a topic that is valid once trimmed (for
example, with trailing or leading spaces) will be rejected by validation even
though `handleSubmit` later trims and saves it, causing the UI to block valid
inputs and behave inconsistently with the saved 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/dashboard/components/gridComponents/Button/ButtonConfigMenuItem.tsx:
##########
@@ -0,0 +1,491 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to You under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { useCallback, useEffect, useMemo, useState } from 'react';
+import { t } from '@superset-ui/core';
+import { styled, css } from '@apache-superset/core/ui';
+import { Button as SupersetButton } from '@superset-ui/core/components/Button';
+import { Form } from '@superset-ui/core/components/Form';
+import { Input, Select, Tooltip } from '@superset-ui/core/components';
+import { Switch } from '@superset-ui/core/components/Switch';
+import { Modal } from '@superset-ui/core/components';
+import { Icons } from '@superset-ui/core/components/Icons';
+import type {
+ ButtonSize,
+ ButtonStyle,
+} from '@superset-ui/core/components/Button/types';
+import type { DashboardButtonMeta } from './types';
+
+const MenuTrigger = styled.div`
+ ${({ theme }) => css`
+ display: flex;
+ align-items: center;
+ justify-content: center;
+ cursor: pointer;
+ color: ${theme.colorTextDescription};
+
+ &:hover {
+ color: ${theme.colorPrimary};
+ }
+ `}
+`;
+
+const ConfigFormWrapper = styled.div`
+ ${({ theme }) => css`
+ width: 320px;
+
+ .ant-form-item {
+ margin-bottom: ${theme.sizeUnit * 2}px;
+ }
+
+ .config-form-footer {
+ display: flex;
+ gap: ${theme.sizeUnit * 2}px;
+ justify-content: flex-end;
+ margin-top: ${theme.sizeUnit * 3}px;
+ }
+ `}
+`;
+
+const TextArea = styled(Input.TextArea)`
+ ${({ theme }) => css`
+ resize: vertical;
+ min-height: ${theme.sizeUnit * 10}px;
+ `}
+`;
+
+// Removed unused constants - using inline options in Select components
+
+export type ButtonConfig = Pick<
+ DashboardButtonMeta,
+ | 'buttonStyle'
+ | 'buttonSize'
+ | 'disabled'
+ | 'tooltip'
+ | 'actionType'
+ | 'url'
+ | 'target'
+ | 'apiEndpoint'
+ | 'apiMethod'
+ | 'apiHeaders'
+ | 'apiPayload'
+ | 'successMessage'
+ | 'errorMessage'
+ | 'confirmBeforeExecute'
+ | 'confirmMessage'
+>;
+
+type FormValues = ButtonConfig;
+
+interface ButtonConfigMenuItemProps {
+ meta: DashboardButtonMeta;
+ onSave: (updates: ButtonConfig) => void;
+ onVisibilityChange?: (open: boolean) => void;
+}
+
+const normalizeButtonSize = (size?: string | null): ButtonSize =>
+ size === 'middle' || !size ? 'default' : (size as ButtonSize);
+
+const normalizeButtonStyle = (style?: string | null): ButtonStyle =>
+ (style as ButtonStyle) || 'primary';
+
+const normalizeActionType = (
+ actionType?: string | null,
+): ButtonConfig['actionType'] =>
+ actionType === 'api' ? 'api' : 'link';
+
+function getInitialValues(meta: DashboardButtonMeta) {
+ return {
+ buttonStyle: normalizeButtonStyle(meta.buttonStyle),
+ buttonSize: normalizeButtonSize(meta.buttonSize),
+ disabled: meta.disabled ?? false,
+ tooltip: meta.tooltip ?? '',
+ actionType: normalizeActionType(meta.actionType),
+ url: meta.url ?? '',
+ target: meta.target ?? '_self',
+ apiEndpoint: meta.apiEndpoint ?? '',
+ apiMethod: (meta.apiMethod ?? 'POST').toUpperCase(),
+ apiHeaders: meta.apiHeaders ?? '',
+ apiPayload: meta.apiPayload ?? '',
+ successMessage:
+ meta.successMessage ?? t('Action executed successfully.'),
+ errorMessage:
+ meta.errorMessage ?? t('Unable to execute action.'),
+ confirmBeforeExecute: meta.confirmBeforeExecute ?? false,
+ confirmMessage:
+ meta.confirmMessage ?? t('Are you sure you want to run this action?'),
+ } satisfies FormValues;
+}
+
+const ButtonConfigMenuItem = ({
+ meta,
+ onSave,
+ onVisibilityChange,
+}: ButtonConfigMenuItemProps) => {
+ const [modalVisible, setModalVisible] = useState(false);
+ const [form] = Form.useForm<FormValues>();
+
+ const initialValues = useMemo(() => getInitialValues(meta), [meta]);
+ const actionOptions = useMemo(
+ () => [
+ { value: 'link', label: t('Open link') },
+ { value: 'api', label: t('Call API') },
+ ],
+ [],
+ );
+ const buttonStyleOptions = useMemo(
+ () => [
+ { value: 'primary', label: t('Primary') },
+ { value: 'secondary', label: t('Secondary') },
+ { value: 'tertiary', label: t('Tertiary') },
+ { value: 'danger', label: t('Danger') },
+ { value: 'link', label: t('Link') },
+ { value: 'dashed', label: t('Dashed') },
+ ],
+ [],
+ );
+ const buttonSizeOptions = useMemo(
+ () => [
+ { value: 'default', label: t('Default') },
+ { value: 'small', label: t('Small') },
+ { value: 'xsmall', label: t('Extra small') },
+ ],
+ [],
+ );
+
+ const [actionType, setActionType] = useState(initialValues.actionType);
+
+ useEffect(() => {
+ if (modalVisible) {
+ form.setFieldsValue(initialValues);
+ setActionType(initialValues.actionType);
+ }
+ }, [form, initialValues, modalVisible]);
+
+ const closeModal = useCallback(
+ (shouldReset = false) => {
+ setModalVisible(false);
+ onVisibilityChange?.(false);
+ if (shouldReset) {
+ form.resetFields();
+ setActionType(initialValues.actionType);
+ }
+ },
+ [form, initialValues.actionType, onVisibilityChange],
+ );
+
+ const handleSubmit = useCallback(
+ async (values: FormValues) => {
+ onSave({
+ ...values,
+ apiMethod: values.apiMethod?.toUpperCase(),
+ url: values.url?.trim() ?? '',
+ apiEndpoint: values.apiEndpoint?.trim() ?? '',
+ apiHeaders: values.apiHeaders?.trim() ?? '',
+ apiPayload: values.apiPayload?.trim() ?? '',
+ tooltip: values.tooltip?.trim() ?? '',
+ successMessage: values.successMessage?.trim() ?? '',
+ errorMessage: values.errorMessage?.trim() ?? '',
+ confirmMessage: values.confirmMessage?.trim() ?? '',
+ });
+ closeModal(false);
+ },
+ [closeModal, onSave],
+ );
+
+ const handleCancel = useCallback(() => {
+ closeModal(true);
+ }, [closeModal]);
+
+ const handleOpenModal = useCallback(() => {
+ setModalVisible(true);
+ onVisibilityChange?.(true);
+ }, [onVisibilityChange]);
+
+ return (
+ <>
+ <Tooltip title={t('Configure button')}>
+ <MenuTrigger
+ onClick={event => {
+ event.stopPropagation();
+ handleOpenModal();
+ }}
+ >
+ <Icons.SettingOutlined iconSize="m" />
+ </MenuTrigger>
+ </Tooltip>
+ <Modal
+ title={t('Button configuration')}
+ show={modalVisible}
+ onHide={handleCancel}
+ footer={
+ <div className="config-form-footer">
+ <SupersetButton
+ buttonStyle="tertiary"
+ buttonSize="small"
+ onClick={handleCancel}
+ >
+ {t('Cancel')}
+ </SupersetButton>
+ <SupersetButton
+ buttonStyle="primary"
+ buttonSize="small"
+ onClick={() => form.submit()}
+ >
+ {t('Save')}
+ </SupersetButton>
+ </div>
+ }
+ >
+ <ConfigFormWrapper data-test="dashboard-button-config">
+ <Form
+ layout="vertical"
+ form={form}
+ initialValues={initialValues}
+ onFinish={handleSubmit}
+ onValuesChange={(changedValues, values) => {
+ if (
+ Object.prototype.hasOwnProperty.call(
+ changedValues,
+ 'actionType',
+ )
+ ) {
+ setActionType(
+ (values.actionType as ButtonConfig['actionType']) ?? 'link',
+ );
+ }
+ }}
+ >
+ <Form.Item<FormValues>
+ name="buttonStyle"
+ label={t('Button style')}
+ >
+ <Select
+ options={buttonStyleOptions}
+ />
+ </Form.Item>
+ <Form.Item<FormValues> name="buttonSize" label={t('Button size')}>
+ <Select
+ options={buttonSizeOptions}
+ />
+ </Form.Item>
+ <Form.Item<FormValues>
+ name="tooltip"
+ label={t('Tooltip')}
+ extra={t('Optional helper text displayed on hover')}
+ >
+ <Input />
+ </Form.Item>
+ <Form.Item<FormValues>
+ name="disabled"
+ label={t('Disable button')}
+ valuePropName="checked"
+ >
+ <Switch />
+ </Form.Item>
+ <Form.Item<FormValues>
+ name="actionType"
+ label={t('Action type')}
+ >
+ <Select options={actionOptions} />
+ </Form.Item>
+ {actionType === 'link' && (
+ <>
+ <Form.Item<FormValues>
+ name="url"
+ label={t('Destination URL')}
+ rules={[
+ {
+ validator: (_, value) => {
+ if (!value || value.trim().length === 0) {
+ return Promise.resolve();
+ }
+ const trimmedValue = value.trim();
+ const trimmedLower = trimmedValue.toLowerCase();
+ // Block dangerous protocols
+ const dangerousProtocols = ['javascript:', 'data:',
'vbscript:', 'file:'];
+ if (dangerousProtocols.some(protocol =>
trimmedLower.startsWith(protocol))) {
+ return Promise.reject(
Review Comment:
**Suggestion:** The list of blocked URL protocols is duplicated in both the
link URL and API endpoint validators, which increases the risk that a future
security update will only change one copy and leave the other vulnerable;
extracting this list into a shared constant used by both validators avoids this
drift. [security]
**Severity Level:** Critical 🚨
```suggestion
const DANGEROUS_PROTOCOLS = ['javascript:', 'data:',
'vbscript:', 'file:'] as const;
// Block dangerous protocols
if (DANGEROUS_PROTOCOLS.some(protocol =>
trimmedLower.startsWith(protocol))) {
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
The same `['javascript:', 'data:', 'vbscript:', 'file:']` array is present
in both the link URL and API endpoint validators with identical semantics, so
this is real duplication in security-sensitive logic. Hoisting it into a shared
constant preserves behavior today while reducing the chance that a future
protocol change gets applied to only one of the two validators. That's a
legitimate code-quality improvement by removing duplicated logic, not just
cosmetic churn.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/src/dashboard/components/gridComponents/Button/ButtonConfigMenuItem.tsx
**Line:** 321:323
**Comment:**
*Security: The list of blocked URL protocols is duplicated in both the
link URL and API endpoint validators, which increases the risk that a future
security update will only change one copy and leave the other vulnerable;
extracting this list into a shared constant used by both validators avoids this
drift.
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]
