codeant-ai-for-open-source[bot] commented on code in PR #40967:
URL: https://github.com/apache/superset/pull/40967#discussion_r3623731856
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/Regular/Scatter/controlPanel.tsx:
##########
@@ -99,114 +428,56 @@ const config: ControlPanelConfig = {
'Size of marker. Also applies to forecast observations.',
),
visibility: ({ controls }: ControlPanelsContainerProps) =>
- Boolean(controls?.markerEnabled?.value),
+ Boolean(controls?.markerEnabled?.value) &&
+ !controls?.size?.value,
},
},
],
- ['zoomable'],
- [minorTicks],
- ...legendSection,
- [<ControlSubSectionHeader>{t('X Axis')}</ControlSubSectionHeader>],
-
[
{
- name: 'x_axis_time_format',
- config: {
- ...sharedControls.x_axis_time_format,
- description: `${D3_TIME_FORMAT_DOCS}.
${TIME_SERIES_DESCRIPTION_TEXT}`,
- visibility: ({ controls }: ControlPanelsContainerProps) =>
- checkColumnType(
- getColumnLabel(controls?.x_axis?.value as QueryFormColumn),
- controls?.datasource?.datasource,
- [GenericDataType.Temporal],
- ),
- disableStash: true,
- resetOnHide: false,
- },
- },
- {
- name: 'x_axis_number_format',
+ name: 'minMarkerSize',
config: {
- ...sharedControls.x_axis_number_format,
- default: '~g',
- mapStateToProps: undefined,
- visibility: ({ controls }: ControlPanelsContainerProps) =>
- checkColumnType(
- getColumnLabel(controls?.x_axis?.value as QueryFormColumn),
- controls?.datasource?.datasource,
- [GenericDataType.Numeric],
- ),
- },
- },
- ],
- [xAxisLabelRotation],
- [xAxisLabelInterval],
- [forceMaxInterval],
- // eslint-disable-next-line react/jsx-key
- ...richTooltipSection,
- // eslint-disable-next-line react/jsx-key
- [<ControlSubSectionHeader>{t('Y Axis')}</ControlSubSectionHeader>],
- ['y_axis_format'],
- ['currency_format'],
- [
- {
- name: 'logAxis',
- config: {
- type: 'CheckboxControl',
- label: t('Logarithmic y-axis'),
- renderTrigger: true,
- default: logAxis,
- description: t('Logarithmic y-axis'),
- },
- },
- ],
- [
- {
- name: 'minorSplitLine',
- config: {
- type: 'CheckboxControl',
- label: t('Minor Split Line'),
- renderTrigger: true,
- default: minorSplitLine,
- description: t('Draw split lines for minor y-axis ticks'),
- },
- },
- ],
- [truncateXAxis],
- [xAxisBounds],
- [
- {
- name: 'truncateYAxis',
- config: {
- type: 'CheckboxControl',
- label: t('Truncate Y Axis'),
- default: truncateYAxis,
+ type: 'SliderControl',
+ label: t('Minimum dot size'),
renderTrigger: true,
+ min: 1,
+ max: 100,
+ default: minMarkerSize,
description: t(
- 'Truncate Y Axis. Can be overridden by specifying a min or max
bound.',
+ 'Size of the dot representing the smallest value of the dot
size metric.',
),
+ visibility: ({ controls }: ControlPanelsContainerProps) =>
+ Boolean(controls?.size?.value),
},
},
- ],
- [
{
- name: 'y_axis_bounds',
+ name: 'maxMarkerSize',
config: {
- type: 'BoundsControl',
- label: t('Y Axis Bounds'),
+ type: 'SliderControl',
+ label: t('Maximum dot size'),
renderTrigger: true,
- default: yAxisBounds,
+ min: 1,
+ max: 100,
+ default: maxMarkerSize,
description: t(
- 'Bounds for the Y-axis. When left empty, the bounds are ' +
- 'dynamically defined based on the min/max of the data. Note
that ' +
- "this feature will only expand the axis range. It won't " +
- "narrow the data's extent.",
+ 'Size of the dot representing the largest value of the dot
size metric.',
),
visibility: ({ controls }: ControlPanelsContainerProps) =>
- Boolean(controls?.truncateYAxis?.value),
+ Boolean(controls?.size?.value),
},
},
],
+ ['zoomable'],
+ [minorTicks],
+ ...legendSection,
+ [<ControlSubSectionHeader>{t('X Axis')}</ControlSubSectionHeader>],
+ ...createAxisControl('x'),
+ [truncateXAxis],
+ [xAxisBounds],
+ [forceMaxInterval],
Review Comment:
**Suggestion:** These controls are always pinned under the X Axis
subsection, so in horizontal orientation they no longer follow the dimension
axis as intended and the panel swap behavior becomes inconsistent. Gate them by
orientation (or move them into the same axis-swapping helper) so they appear
under the correct subsection. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Axis truncation and bounds controls fixed under X Axis.
- ⚠️ Horizontal orientation shows dimension controls split across
subsections.
- ⚠️ UX diverges from Bar chart axis orientation behavior.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. In
`superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/Regular/Scatter/controlPanel.tsx:361-379`,
the `orientation` radio control is defined, with values
`OrientationType.Vertical` and
`OrientationType.Horizontal`. This orientation is later used by
`isHorizontal`/`isVertical` and `createAxisControl` to swap axis-specific
controls.
2. At lines 162-320, `createAxisControl(axis: 'x' | 'y')` defines axis
controls, using
`showsDimensionAxis` and `showsMetricAxis` so that many controls (time
format, number
format, label rotation, logAxis, truncateYAxis, y_axis_bounds, etc.) appear
under either
the X Axis or Y Axis subsection depending on `orientation`.
3. At lines 473-481, the Chart Options section composes the subsections: it
adds the X
Axis header, then `...createAxisControl('x')`, then the rows
`[truncateXAxis],
[xAxisBounds], [forceMaxInterval], ...richTooltipSection`, followed by the Y
Axis header
and `...createAxisControl('y')`. The three rows for `truncateXAxis`,
`xAxisBounds`, and
`forceMaxInterval` are statically placed under the X Axis subsection.
4. When the control panel is rendered and the user switches to horizontal
orientation
(dimension on Y axis, metric on X axis as described in the PR), the
dimension/metric axis
controls created inside `createAxisControl` swap between X and Y
subsections, but
`truncateXAxis`, `xAxisBounds`, and `forceMaxInterval` remain under X Axis
only, causing
these axis-bound controls to be grouped with the wrong subsection relative
to the
dimension axis and making the orientation-dependent panel layout
inconsistent.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=0e2481be2bc34c419a4e0e369e4e3741&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=0e2481be2bc34c419a4e0e369e4e3741&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<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-echarts/src/Timeseries/Regular/Scatter/controlPanel.tsx
**Line:** 475:477
**Comment:**
*Logic Error: These controls are always pinned under the X Axis
subsection, so in horizontal orientation they no longer follow the dimension
axis as intended and the panel swap behavior becomes inconsistent. Gate them by
orientation (or move them into the same axis-swapping helper) so they appear
under the correct subsection.
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.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40967&comment_hash=69050c4c4b6f3876b2c6a03da4b8964c7b12b1683fd87923f6e577366981d354&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40967&comment_hash=69050c4c4b6f3876b2c6a03da4b8964c7b12b1683fd87923f6e577366981d354&reaction=dislike'>👎</a>
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/Regular/Scatter/controlPanel.tsx:
##########
@@ -51,18 +56,342 @@ const {
logAxis,
markerEnabled,
markerSize,
+ maxMarkerSize,
+ minMarkerSize,
minorSplitLine,
+ orientation,
rowLimit,
truncateYAxis,
yAxisBounds,
} = DEFAULT_FORM_DATA;
+
+const isHorizontal = (controls: ControlStateMapping) =>
+ controls?.orientation?.value === OrientationType.Horizontal;
+const isVertical = (controls: ControlStateMapping) => !isHorizontal(controls);
+
+function createAxisTitleControl(axis: 'x' | 'y'): ControlSetRow[] {
+ const isXAxis = axis === 'x';
+ return [
+ [
+ {
+ name: 'x_axis_title',
+ config: {
+ type: 'TextControl',
+ label: t('Axis Title'),
+ renderTrigger: true,
+ default: '',
+ visibility: ({ controls }: ControlPanelsContainerProps) =>
+ isXAxis ? isVertical(controls) : isHorizontal(controls),
+ disableStash: true,
+ resetOnHide: false,
+ },
+ },
+ ],
+ [
+ {
+ name: 'x_axis_title_margin',
+ config: {
+ type: 'SelectControl',
+ freeForm: true,
+ clearable: true,
+ label: t('Axis title margin'),
+ renderTrigger: true,
+ default: sections.TITLE_MARGIN_OPTIONS[3],
+ choices: formatSelectOptions(sections.TITLE_MARGIN_OPTIONS),
+ visibility: ({ controls }: ControlPanelsContainerProps) =>
+ isXAxis ? isVertical(controls) : isHorizontal(controls),
+ disableStash: true,
+ resetOnHide: false,
+ },
+ },
+ ],
+ [
+ {
+ name: 'y_axis_title',
+ config: {
+ type: 'TextControl',
+ label: t('Axis Title'),
+ renderTrigger: true,
+ default: '',
+ visibility: ({ controls }: ControlPanelsContainerProps) =>
+ isXAxis ? isHorizontal(controls) : isVertical(controls),
+ disableStash: true,
+ resetOnHide: false,
+ },
+ },
+ ],
+ [
+ {
+ name: 'y_axis_title_margin',
+ config: {
+ type: 'SelectControl',
+ freeForm: true,
+ clearable: true,
+ label: t('Axis title margin'),
+ renderTrigger: true,
+ default: sections.TITLE_MARGIN_OPTIONS[4],
+ choices: formatSelectOptions(sections.TITLE_MARGIN_OPTIONS),
+ visibility: ({ controls }: ControlPanelsContainerProps) =>
+ isXAxis ? isHorizontal(controls) : isVertical(controls),
+ disableStash: true,
+ resetOnHide: false,
+ },
+ },
+ ],
+ [
+ {
+ name: 'y_axis_title_position',
+ config: {
+ type: 'SelectControl',
+ freeForm: true,
+ clearable: false,
+ label: t('Axis title position'),
+ renderTrigger: true,
+ default: sections.TITLE_POSITION_OPTIONS[0][0],
+ choices: sections.TITLE_POSITION_OPTIONS,
+ visibility: ({ controls }: ControlPanelsContainerProps) =>
+ isXAxis ? isHorizontal(controls) : isVertical(controls),
+ disableStash: true,
+ resetOnHide: false,
+ },
+ },
+ ],
+ ];
+}
+
+function createAxisControl(axis: 'x' | 'y'): ControlSetRow[] {
+ const isXAxis = axis === 'x';
+ // The dimension (x_axis) controls follow the dimension axis: they show
+ // under "X Axis" when vertical and under "Y Axis" when horizontal. The
+ // metric controls follow the opposite axis.
+ const showsDimensionAxis = (controls: ControlStateMapping) =>
+ isXAxis ? isVertical(controls) : isHorizontal(controls);
+ const showsMetricAxis = (controls: ControlStateMapping) =>
+ isXAxis ? isHorizontal(controls) : isVertical(controls);
+ return [
+ [
+ {
+ name: 'x_axis_time_format',
+ config: {
+ ...sharedControls.x_axis_time_format,
+ default: 'smart_date',
+ description: `${D3_TIME_FORMAT_DOCS}.
${TIME_SERIES_DESCRIPTION_TEXT}`,
+ visibility: ({ controls }: ControlPanelsContainerProps) =>
+ showsDimensionAxis(controls) &&
+ checkColumnType(
+ getColumnLabel(controls?.x_axis?.value as QueryFormColumn),
+ controls?.datasource?.datasource,
+ [GenericDataType.Temporal],
+ ),
+ disableStash: true,
+ resetOnHide: false,
+ },
+ },
+ ],
+ [
+ {
+ name: 'x_axis_number_format',
+ config: {
+ ...sharedControls.x_axis_number_format,
+ default: '~g',
+ mapStateToProps: undefined,
+ visibility: ({ controls }: ControlPanelsContainerProps) =>
+ showsDimensionAxis(controls) &&
+ checkColumnType(
+ getColumnLabel(controls?.x_axis?.value as QueryFormColumn),
+ controls?.datasource?.datasource,
+ [GenericDataType.Numeric],
+ ),
+ disableStash: true,
+ resetOnHide: false,
+ },
+ },
+ ],
+ [
+ {
+ name: xAxisLabelRotation.name,
+ config: {
+ ...xAxisLabelRotation.config,
+ visibility: ({ controls }: ControlPanelsContainerProps) =>
+ showsDimensionAxis(controls),
+ disableStash: true,
+ resetOnHide: false,
+ },
+ },
+ ],
+ [
+ {
+ name: xAxisLabelInterval.name,
+ config: {
+ ...xAxisLabelInterval.config,
+ visibility: ({ controls }: ControlPanelsContainerProps) =>
+ showsDimensionAxis(controls),
+ disableStash: true,
+ resetOnHide: false,
+ },
+ },
+ ],
+ [
+ {
+ name: 'y_axis_format',
+ config: {
+ ...sharedControls.y_axis_format,
+ label: t('Axis Format'),
+ visibility: ({ controls }: ControlPanelsContainerProps) =>
+ showsMetricAxis(controls),
+ disableStash: true,
+ resetOnHide: false,
+ },
+ },
+ ],
+ ['currency_format'],
Review Comment:
**Suggestion:** The currency format control is added unconditionally inside
a helper that is rendered for both axis sections, so the same control will
appear twice and can be shown in the wrong section for a given orientation.
Make this control orientation-aware (metric-axis only) or render it only once.
[logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Scatter chart currency format control duplicated per axis section.
- ⚠️ Users may confuse which axis currency applies.
- ⚠️ Inconsistent with metric-axis-only currency format elsewhere.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. In
`superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/Regular/Scatter/controlPanel.tsx:162-320`,
note that `createAxisControl(axis: 'x' | 'y')` returns an array of
`ControlSetRow`s used
for both X and Y axis subsections.
2. Inside `createAxisControl` at line 247, the row `['currency_format'],` is
included
without any `visibility` logic tied to `showsMetricAxis` or
`showsDimensionAxis`, unlike
surrounding metric/dimension axis controls.
3. Later in the same file at lines 473-480, the config section adds both
subsections:
`[<ControlSubSectionHeader>{t('X Axis')}</ControlSubSectionHeader>],
...createAxisControl('x'), ...` and `[<ControlSubSectionHeader>{t('Y
Axis')}</ControlSubSectionHeader>], ...createAxisControl('y'),` meaning
`createAxisControl` (and thus the `currency_format` row) is rendered twice.
4. When the scatter chart plugin renders this control panel (via the
chart-controls
framework that consumes `config` from this file), the `currency_format`
control appears in
both X Axis and Y Axis sections, even though it conceptually belongs to the
metric axis
only, leading to duplicated UI and ambiguous placement.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=edf05a10ab4240c68eb7c8fedf48309c&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=edf05a10ab4240c68eb7c8fedf48309c&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<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-echarts/src/Timeseries/Regular/Scatter/controlPanel.tsx
**Line:** 247:247
**Comment:**
*Logic Error: The currency format control is added unconditionally
inside a helper that is rendered for both axis sections, so the same control
will appear twice and can be shown in the wrong section for a given
orientation. Make this control orientation-aware (metric-axis only) or render
it only once.
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.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40967&comment_hash=00c051b132c8fe85d6cd9576be71470c17dcfd21f73c3abf5601e55a6568bc97&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40967&comment_hash=00c051b132c8fe85d6cd9576be71470c17dcfd21f73c3abf5601e55a6568bc97&reaction=dislike'>👎</a>
##########
superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/Area/controlPanel.test.ts:
##########
@@ -16,58 +16,14 @@
* specific language governing permissions and limitations
* under the License.
*/
-import { ControlPanelsContainerProps } from
'@superset-ui/chart-controls/types';
import { GenericDataType } from '@apache-superset/core/common';
import controlPanel from '../../../src/Timeseries/Area/controlPanel';
+import { getControl, mockControls } from '../helpers';
Review Comment:
**Suggestion:** `mockControls` is imported but never used, which introduces
dead code and can fail strict lint rules for unused imports. Remove the unused
import or use it in assertions. [possible bug]
<details>
<summary><b>Severity Level:</b> Minor 🧹</summary>
```mdx
- ⚠️ Test file triggers unused-import lint warnings or errors.
- ⚠️ CI pipeline may fail on style violations.
- ⚠️ Minor noise and clutter in test codebase.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open
`superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/Area/controlPanel.test.ts`.
At line 21, observe the import `import { getControl, mockControls } from
'../helpers';`.
2. At lines 25-27, `timeFormatControl` and `numberFormatControl` are
initialized using
`getControl(config, 'x_axis_time_format')` and `getControl(config,
'x_axis_number_format')`, respectively; `mockControls` is not referenced
anywhere else in
the file.
3. Search within the same file for `mockControls` usage; no matches are
found, confirming
that the imported symbol is unused.
4. Running tooling that flags unused imports (e.g., ESLint or TypeScript
with appropriate
rules configured in this repository) will report `mockControls` as an unused
import in
this test file, causing lint warnings or failures in the standard dev/CI
workflow.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=d2624d92d6e84d6bb2795d71df6ceb07&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=d2624d92d6e84d6bb2795d71df6ceb07&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<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-echarts/test/Timeseries/Area/controlPanel.test.ts
**Line:** 21:21
**Comment:**
*Possible Bug: `mockControls` is imported but never used, which
introduces dead code and can fail strict lint rules for unused imports. Remove
the unused import or use it in assertions.
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.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40967&comment_hash=ca983054736a9e0c2d899703b655093a4149d5b563fff7e34d11498728c8c9c4&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40967&comment_hash=ca983054736a9e0c2d899703b655093a4149d5b563fff7e34d11498728c8c9c4&reaction=dislike'>👎</a>
--
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]
