You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "raja-naveed (via GitHub)" <gi...@apache.org> on 2025/12/03 06:05:57 UTC
[PR] Customized Layout Elements button and 3d modal [superset]
raja-naveed opened a new pull request, #36390:
URL: https://github.com/apache/superset/pull/36390
<!---
Please write the PR title following the conventions at https://www.conventionalcommits.org/en/v1.0.0/
Example:
fix(dashboard): load charts correctly
-->
### SUMMARY
<!--- Describe the change below, including rationale and design decisions -->
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
<!--- Skip this if not applicable -->
### TESTING INSTRUCTIONS
<!--- Required! What steps can be taken to manually verify the changes? -->
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [ ] Has associated issue:
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] Customized Layout Elements button and 3d modal [superset]
Posted by "sadpandajoe (via GitHub)" <gi...@apache.org>.
sadpandajoe closed pull request #36390: Customized Layout Elements button and 3d modal
URL: https://github.com/apache/superset/pull/36390
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] Customized Layout Elements button and 3d modal [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] Customized Layout Elements button and 3d modal [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #36390:
URL: https://github.com/apache/superset/pull/36390#issuecomment-3609238165
Aside from the issues the bots caught, there seem to be a LOT of `.md` files here - and no explanation of what the PR is intending to solve. If you can help us by filling in the PR description, maybe we can start helping from there.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] Customized Layout Elements button and 3d modal [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on PR #36390:
URL: https://github.com/apache/superset/pull/36390#issuecomment-3606529531
CodeAnt AI is running Incremental review
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] Customized Layout Elements button and 3d modal [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on PR #36390:
URL: https://github.com/apache/superset/pull/36390#issuecomment-3605270547
## Nitpicks 🔍
<table>
<tr><td>🔒 <strong>No security issues identified</strong></td></tr>
<tr><td>⚡ <strong>Recommended areas for review</strong><br><br>
- [ ] <a href='https://github.com/apache/superset/pull/36390/files#diff-bfad2751d38e1a043dc3ad8c140fc2e4cdf8345c0df42c744edbdd5e721d29cbR44-R48'><strong>Security Issue</strong></a><br>MQTT broker URL, username, and password are hard-coded in a frontend bundle, which effectively exposes broker credentials to any user of the dashboard. These values should be externalized and protected (e.g., via backend proxy or server-side config) rather than shipped to the browser.<br>
- [ ] <a href='https://github.com/apache/superset/pull/36390/files#diff-90a7083a20beba74b4e1f11ab248fba1ab6466b08f167cee3d092c727dcc0b68R149-R153'><strong>Sensitive Data</strong></a><br>MQTT broker URL, username, and especially password are hard-coded in frontend code, meaning they will be shipped to every browser and can be trivially extracted. The reviewer should confirm this is acceptable for the deployment or move these values to a secure backend or configuration layer.<br>
- [ ] <a href='https://github.com/apache/superset/pull/36390/files#diff-ec4bd7cb15a3170ce228455f2d838ae95f87c9efc9e3ff24fb5f4ace29b599ecR37-R39'><strong>Sensitive Config</strong></a><br>The `DashboardAlertsMeta` type includes MQTT credentials (`mqttUsername`, `mqttPassword`) as part of the dashboard layout metadata, which is typically persisted and sent to the browser. This may unintentionally expose broker credentials to all users who can view the dashboard. The reviewer should validate whether credentials are truly needed on the client and, if so, whether they are handled via a safer mechanism (e.g., server-side proxy) instead of storing them in layout meta.<br>
- [ ] <a href='https://github.com/apache/superset/pull/36390/files#diff-d44f4aab90f11377026bb11846aa4b4b971b3b88f644c3a1cb40752847412fe2R429-R451'><strong>Validation</strong></a><br>The `apiHeaders` and `apiPayload` fields are described and documented as JSON but are currently free-form strings without any validation. If invalid JSON is saved, downstream code that parses these fields may throw at execution time rather than surfacing errors in the configuration form. It would be safer to add JSON validation rules in the form so misconfigurations are caught early.<br>
- [ ] <a href='https://github.com/apache/superset/pull/36390/files#diff-68eae70bc67f0004ba108d71b4a77c4e9c3d5d883d69acbf4d003147b2f80cfcR185-R196'><strong>Possible Bug</strong></a><br>MQTT topic validation uses the raw `value` for the wildcard `#` position check without trimming whitespace first. This means inputs like `sensors/# ` (with trailing space) are rejected even though they would be saved as a valid trimmed topic, which can confuse users or block valid submissions.<br>
- [ ] <a href='https://github.com/apache/superset/pull/36390/files#diff-e771c54a5f4f6c95be03d98683162a3e4973c8def1ef71013de0f40778885196R288-R307'><strong>Button UX</strong></a><br>The button is disabled whenever configuration is missing, regardless of `editMode`. In edit mode this may prevent interacting with the button content (e.g., editing the title via `EditableTitle`) until configuration is complete, which could be confusing or block basic layout editing. Consider whether the disabled state should be applied only in view mode.<br>
- [ ] <a href='https://github.com/apache/superset/pull/36390/files#diff-aa2ede5fe8e8a857ca900ccc27578e88779290b31efbd7cc61677d3d4b780e89R133-R208'><strong>SSRF Risk</strong></a><br>The new `/v1/proxy/` endpoint acts as a generic outbound HTTP proxy for any authenticated user without host or IP restrictions, which can introduce a Server-Side Request Forgery (SSRF) risk (e.g., access to internal services or cloud metadata endpoints). This should be reviewed to ensure appropriate allowlists/denylists, IP/port checks, and other mitigations are in place before exposing it.<br>
- [ ] <a href='https://github.com/apache/superset/pull/36390/files#diff-aa2ede5fe8e8a857ca900ccc27578e88779290b31efbd7cc61677d3d4b780e89R133-R149'><strong>Access Control</strong></a><br>The `proxy_external_api` endpoint does not use the `@has_access_api` decorator while other API endpoints do, and instead manually checks `g.user`. This may bypass existing permission/role checks or API token flows that `has_access_api` enforces. Confirm that this endpoint is intentionally less restricted and that it aligns with Superset's standard API authorization model.<br>
- [ ] <a href='https://github.com/apache/superset/pull/36390/files#diff-259a7a48fa7bbdb6edd62a32c514db47e0625e2bb8504d012b4fcf31915be408R118-R153'><strong>Security Issue</strong></a><br>The script prints the generated Superset secret key to stdout and uses a very weak default admin password (`admin`). This can leak sensitive information in shared terminals or logs and encourages insecure defaults for production deployments.<br>
- [ ] <a href='https://github.com/apache/superset/pull/36390/files#diff-e61bfb812546b11eb8feefa1aa68d678039748cbd90ea867bc23863cdc11e7abR146-R175'><strong>Possible Bug</strong></a><br>Database initialization detection uses `SELECT 1` to decide whether to run `superset db upgrade` and `superset init`. A fresh PostgreSQL instance with the `superset` database created will still return success for `SELECT 1`, so initial migrations and setup may never run, leaving Superset uninitialized even though the script thinks the database is ready.<br>
- [ ] <a href='https://github.com/apache/superset/pull/36390/files#diff-259a7a48fa7bbdb6edd62a32c514db47e0625e2bb8504d012b4fcf31915be408R30-R35'><strong>Possible Bug</strong></a><br>The Docker Compose availability check allows systems that only have the legacy `docker-compose` binary to pass, but the rest of the script always uses `docker compose`. On such systems, later `docker compose` commands will fail at runtime even though the initial check succeeded. Consider normalizing to one command via a variable and using it consistently.<br>
- [ ] <a href='https://github.com/apache/superset/pull/36390/files#diff-e61bfb812546b11eb8feefa1aa68d678039748cbd90ea867bc23863cdc11e7abR38-R201'><strong>Possible Bug</strong></a><br>The script allows either `docker compose` or `docker-compose` to satisfy the dependency check, but all subsequent calls hard-code `docker compose`. On systems that only have `docker-compose` installed, the check will pass but every `docker compose ...` invocation will fail at runtime.<br>
- [ ] <a href='https://github.com/apache/superset/pull/36390/files#diff-6d70f30b3f4c341ec80a1ffb6e382a5174c0625d239a57b778a34cbdb5b3116bR24-R33'><strong>Possible Bug</strong></a><br>The `grep` check for `@google/model-viewer` does not distinguish between "string not found" and "package.json missing" (or other `grep` errors), so a missing `superset-frontend/package.json` will surface only as a non-blocking warning instead of a clear hard failure, which can hide misconfiguration in CI or new environments.<br>
- [ ] <a href='https://github.com/apache/superset/pull/36390/files#diff-a6852a68ced1f6179f15ea996abdfbfaa3c3d3839bec685312a91810cbfabab0R67-R85'><strong>Possible Bug</strong></a><br>The default metadata for `BUTTON_TYPE` sets `apiHeaders` and `apiPayload` to empty strings. If the consuming code expects these fields to be objects (e.g., parsed JSON or key/value maps) rather than plain strings, this mismatch could lead to runtime errors when reading, merging, or serializing these values. The reviewer should confirm that downstream usage of these fields correctly handles string defaults or adjust the defaults to match the expected type.<br>
- [ ] <a href='https://github.com/apache/superset/pull/36390/files#diff-8f78f3866f02305f1df51579c939174b2c53e8295ad13bd9d2ab763176a792c1R29-R47'><strong>Sensitive Credentials</strong></a><br>Non-default database passwords are hardcoded into the committed environment file (e.g., DATABASE_PASSWORD and POSTGRES_PASSWORD). These may be reused elsewhere and could represent real credentials, so they should be externalized or replaced with clearly dummy values and overridden locally.<br>
- [ ] <a href='https://github.com/apache/superset/pull/36390/files#diff-8f78f3866f02305f1df51579c939174b2c53e8295ad13bd9d2ab763176a792c1R27-R60'><strong>Connectivity Configuration</strong></a><br>The database and Redis hosts are switched to host.docker.internal with nonstandard ports, which will break the default docker-compose network assumptions unless corresponding services run on the host. This may prevent the stack from starting or cause confusing connection errors for other developers.<br>
- [ ] <a href='https://github.com/apache/superset/pull/36390/files#diff-a327f3e78ad99f98e52a27334944cad1970aeb8034539bd45bb07e6644af72beR21-R29'><strong>Possible Bug</strong></a><br>The current image-existence check using `docker images %IMAGE_TAG%` likely never fails even when the image is missing, since `docker images` usually returns a zero exit code with empty results. This may cause the script to proceed to `docker save` and only fail later, resulting in a confusing user experience and unreliable validation of the image tag.<br>
- [ ] <a href='https://github.com/apache/superset/pull/36390/files#diff-456e6ee553102f09ece397bff6e57bc2fbc8be138feea8be81044220f6a72972R343-R362'><strong>Documentation Bug</strong></a><br>The documented IoT endpoint domain is inconsistent: earlier it uses `https://app.snap4idcity.com`, while the note block references `https://app.snap4idtcity.com` (extra `t`). This mismatch can confuse users and should be corrected to the intended domain.<br>
- [ ] <a href='https://github.com/apache/superset/pull/36390/files#diff-734e56652f886b4c194799a7b3640a1f06c50dd1200918d2b0a453d38647203bR345-R361'><strong>Possible Bug</strong></a><br>The example external API hostname (`https://app.snap4idcity.com`) does not match the hostname in the note block (`https://app.snap4idtcity.com`), which likely contains a typo and may lead users to configure the wrong domain in their CSP.<br>
</td></tr>
</table>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] Customized Layout Elements button and 3d modal [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #36390:
URL: https://github.com/apache/superset/pull/36390#issuecomment-3605241787
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] Customized Layout Elements button and 3d modal [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on PR #36390:
URL: https://github.com/apache/superset/pull/36390#issuecomment-3610577689
CodeAnt AI is running Incremental review
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] Customized Layout Elements button and 3d modal [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on PR #36390:
URL: https://github.com/apache/superset/pull/36390#issuecomment-3605241588
CodeAnt AI is reviewing your PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org