codeant-ai-for-open-source[bot] commented on code in PR #40967:
URL: https://github.com/apache/superset/pull/40967#discussion_r3394255806
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/Regular/Scatter/controlPanel.tsx:
##########
@@ -99,115 +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,
Review Comment:
**Suggestion:** The fixed marker size control is hidden whenever a dot-size
metric is selected, but the renderer still uses `markerSize` as the fallback
for points without a size value. This makes fallback sizing effectively
unconfigurable in the intended size-metric mode and leaves null/shifted points
stuck at an implicit default. Keep a configurable fallback-size control visible
when `size` is set (or add a dedicated fallback size control). [incomplete
implementation]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Scatter charts with size metrics cannot tune fallback marker size.
- ⚠️ Null or time-shifted points render at unintended default size.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Create or open an ECharts Timeseries Scatter chart (`viz_type:
'echarts_timeseries_scatter'`, see
`superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/Scatter/transformProps.test.ts:43`)
which uses the control panel defined in
`superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/Regular/Scatter/controlPanel.tsx`.
2. In the Query section, set a "Dot size metric" via the `size` control
defined at
`controlPanel.tsx:323-336`; this populates `formData.size` and makes
`controls.size.value`
truthy in the control panel state.
3. Observe that in the Chart Options section, the "Marker Size" slider
(`markerSize`
control at `controlPanel.tsx:419-427`) becomes hidden because its
`visibility` function at
`controlPanel.tsx:430-432` requires both `markerEnabled` and
`!controls?.size?.value`,
which is false once a size metric is selected.
4. At render time, the chart uses `markerSize` as a runtime fallback for
points without a
numeric size value:
`superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts:237-247`
defines `symbolSizeFn`, which returns `markerSize` whenever `sizeValue` is
not a finite
number. With the `markerSize` control hidden in size-metric mode, users
cannot adjust this
fallback size while configuring dot-size metrics, so null/derived points
remain stuck at
the implicit default or a previously-set value that is no longer visible.
```
</details>
[Fix in
Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=2c757ad8e9e34c93b1dbd0829ffc40a5&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
| [Fix in VSCode
Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=2c757ad8e9e34c93b1dbd0829ffc40a5&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:** 430:432
**Comment:**
*Incomplete Implementation: The fixed marker size control is hidden
whenever a dot-size metric is selected, but the renderer still uses
`markerSize` as the fallback for points without a size value. This makes
fallback sizing effectively unconfigurable in the intended size-metric mode and
leaves null/shifted points stuck at an implicit default. Keep a configurable
fallback-size control visible when `size` is set (or add a dedicated fallback
size control).
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=2360b0e4c11b0485f5336260956bc6d20bdc6d91bc0c1409d430c05c802d8d70&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40967&comment_hash=2360b0e4c11b0485f5336260956bc6d20bdc6d91bc0c1409d430c05c802d8d70&reaction=dislike'>👎</a>
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/Regular/Scatter/controlPanel.tsx:
##########
@@ -99,115 +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,
- default: 'smart_date',
- 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),
Review Comment:
**Suggestion:** `minMarkerSize` and `maxMarkerSize` are independent sliders
with no cross-field validation, so users can save `min > max`, which inverts
the size mapping and contradicts the "minimum/maximum" semantics. Add
validation (or normalization) to enforce `minMarkerSize <= maxMarkerSize`.
[incorrect condition logic]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Dot-size scaling reverses when user sets min > max.
- ⚠️ "Minimum/maximum dot size" labels no longer match rendering.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open an ECharts Timeseries Scatter chart (`viz_type:
'echarts_timeseries_scatter'`,
tested in `test/Timeseries/Scatter/transformProps.test.ts`) using the
control panel
defined in
`superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/Regular/Scatter/controlPanel.tsx`.
2. Select a "Dot size metric" so that the `size` control at
`controlPanel.tsx:323-336` is
populated; this makes the `minMarkerSize` and `maxMarkerSize` sliders
visible due to the
`visibility` conditions at `controlPanel.tsx:449-450` and
`controlPanel.tsx:465-466` which
only check `Boolean(controls?.size?.value)`.
3. In the Chart Options section, drag "Minimum dot size" above "Maximum dot
size" (e.g.,
minMarkerSize = 40, maxMarkerSize = 10). There are no `validators` or
cross-field checks
on these controls at `controlPanel.tsx:438-452` and `454-467`, so `formData`
can be saved
with `minMarkerSize > maxMarkerSize`.
4. When the chart renders,
`superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts:97-99`
constructs `markerSizeRange: [number, number] = [minMarkerSize,
maxMarkerSize]` and passes
it to `getAreaScaledSymbolSize` via `symbolSizeFn` at
`transformProps.ts:237-247`. With an
inverted range ([40, 10]), `getAreaScaledSymbolSize`
(`utils/series.ts:15-31`) still
returns values between 10 and 40, but the mapping is reversed (smallest
metric gets the
largest diameter, largest metric the smallest), contradicting the
"Minimum/Maximum dot
size" semantics and producing visually incorrect scaling.
```
</details>
[Fix in
Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=f8790d723ec243579ee83872ad79a808&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
| [Fix in VSCode
Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=f8790d723ec243579ee83872ad79a808&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:** 438:466
**Comment:**
*Incorrect Condition Logic: `minMarkerSize` and `maxMarkerSize` are
independent sliders with no cross-field validation, so users can save `min >
max`, which inverts the size mapping and contradicts the "minimum/maximum"
semantics. Add validation (or normalization) to enforce `minMarkerSize <=
maxMarkerSize`.
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=8a90534a2e5aa4c1a1a38a33253038b526cea7cdc1cc7a584dd4432ca5557d7f&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40967&comment_hash=8a90534a2e5aa4c1a1a38a33253038b526cea7cdc1cc7a584dd4432ca5557d7f&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]
