codeant-ai-for-open-source[bot] commented on code in PR #40967:
URL: https://github.com/apache/superset/pull/40967#discussion_r3400608057
##########
superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/Scatter/transformProps.test.ts:
##########
@@ -175,3 +175,283 @@ describe('Scatter Chart X-axis Number Formatting', () => {
},
);
});
+
+describe('Scatter Chart Orientation and Dot Size Metric', () => {
+ const baseFormData: EchartsTimeseriesFormData = {
+ ...DEFAULT_FORM_DATA,
+ colorScheme: 'supersetColors',
+ datasource: '1__table',
+ metrics: ['sum_val'],
+ x_axis: 'category_col',
+ groupby: [],
+ viz_type: 'echarts_timeseries_scatter',
+ seriesType: EchartsTimeseriesSeriesType.Scatter,
+ };
+
+ const categoricalData = [
+ {
+ data: [
+ { category_col: 'A', sum_val: 1, size_metric: 10 },
+ { category_col: 'B', sum_val: 2, size_metric: 25 },
+ { category_col: 'C', sum_val: 3, size_metric: 40 },
+ ],
+ colnames: ['category_col', 'sum_val', 'size_metric'],
+ coltypes: [
+ GenericDataType.String,
+ GenericDataType.Numeric,
+ GenericDataType.Numeric,
+ ],
+ label_map: {
+ category_col: ['category_col'],
+ sum_val: ['sum_val'],
+ size_metric: ['size_metric'],
+ },
+ },
+ ];
+
+ const baseChartPropsConfig = {
+ width: 800,
+ height: 600,
+ theme: supersetTheme,
+ };
+
+ const getScatterSeries = (props: ReturnType<typeof transformProps>) =>
+ (props.echartOptions.series as any[]).filter(s => s.type === 'scatter');
+
+ const singleMetricData = [
+ {
+ ...categoricalData[0],
+ data: categoricalData[0].data.map(row => ({
+ category_col: row.category_col,
+ sum_val: row.sum_val,
+ })),
+ colnames: ['category_col', 'sum_val'],
+ coltypes: [GenericDataType.String, GenericDataType.Numeric],
+ },
+ ];
+
+ test('horizontal orientation swaps the dimension axis onto the y-axis', ()
=> {
+ const chartProps = new ChartProps({
+ ...baseChartPropsConfig,
+ queriesData: singleMetricData,
+ formData: { ...baseFormData, orientation: 'horizontal' },
+ });
+
+ const transformedProps = transformProps(
+ chartProps as EchartsTimeseriesChartProps,
+ );
+ const { xAxis, yAxis } = transformedProps.echartOptions as any;
+ expect(yAxis.type).toBe('category');
+ expect(xAxis.type).toBe('value');
+
+ const series = getScatterSeries(transformedProps);
+ expect(series).toHaveLength(1);
+ // data points are flipped to [metric, dimension]
+ expect(series[0].data[0]).toEqual([1, 'A']);
+ });
+
+ test('vertical orientation keeps the dimension axis on the x-axis', () => {
+ const chartProps = new ChartProps({
+ ...baseChartPropsConfig,
+ queriesData: singleMetricData,
+ formData: baseFormData,
+ });
+
+ const transformedProps = transformProps(
+ chartProps as EchartsTimeseriesChartProps,
+ );
+ const { xAxis, yAxis } = transformedProps.echartOptions as any;
Review Comment:
**Suggestion:** Remove the second `any` cast for axis destructuring by
typing the chart options object with an explicit ECharts options interface.
[custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
This is another direct `any` cast in TypeScript test code. The custom rule
requires avoiding `any`, so the violation is real.
</details>
[Fix in
Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=c3606d0397594e199df03b4d20e79bb9&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=c3606d0397594e199df03b4d20e79bb9&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/Scatter/transformProps.test.ts
**Line:** 263:263
**Comment:**
*Custom Rule: Remove the second `any` cast for axis destructuring by
typing the chart options object with an explicit ECharts options interface.
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=c01420d9b1bb9952dd1d6edca3144fd7f0455bd78aab0d54914fa7e8a3fe5a9d&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40967&comment_hash=c01420d9b1bb9952dd1d6edca3144fd7f0455bd78aab0d54914fa7e8a3fe5a9d&reaction=dislike'>👎</a>
##########
superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/Scatter/controlPanel.test.ts:
##########
@@ -148,3 +148,59 @@ test('x_axis_number_format control should be hidden for
non-numeric data types',
expect(isNumberVisible(null, null)).toBe(false);
expect(isNumberVisible('time_column', GenericDataType.Temporal)).toBe(false);
});
+
+// tests for orientation and dot size controls
+const orientationControl: any = getControl('orientation');
+const sizeControl: any = getControl('size');
+const minMarkerSizeControl: any = getControl('minMarkerSize');
+const maxMarkerSizeControl: any = getControl('maxMarkerSize');
+
+test('scatter chart control panel should include an orientation control
defaulting to vertical', () => {
+ expect(orientationControl).toBeDefined();
+ expect(orientationControl.config.default).toBe('vertical');
+ expect(orientationControl.config.options).toEqual([
+ ['vertical', expect.anything()],
+ ['horizontal', expect.anything()],
+ ]);
+});
+
+test('scatter chart control panel should include an optional dot size metric
control', () => {
+ expect(sizeControl).toBeDefined();
+ expect(sizeControl.config.validators).toEqual([]);
+ expect(sizeControl.config.default).toBeNull();
+});
+
+const mockSizeControls = (
+ sizeValue: string | null,
+): ControlPanelsContainerProps =>
+ ({
+ controls: {
+ size: { value: sizeValue },
+ markerEnabled: { value: true },
+ },
+ }) as unknown as ControlPanelsContainerProps;
+
+test('dot size range controls should only be visible when a size metric is
set', () => {
+ expect(minMarkerSizeControl.config.visibility(mockSizeControls(null))).toBe(
+ false,
+ );
+ expect(maxMarkerSizeControl.config.visibility(mockSizeControls(null))).toBe(
+ false,
+ );
+ expect(
+ minMarkerSizeControl.config.visibility(mockSizeControls('size_metric')),
+ ).toBe(true);
+ expect(
+ maxMarkerSizeControl.config.visibility(mockSizeControls('size_metric')),
+ ).toBe(true);
+});
+
+test('fixed marker size control should hide when a size metric is set', () => {
+ const markerSizeControl: any = getControl('markerSize');
Review Comment:
**Suggestion:** Change this `any` usage to the same concrete control type
used elsewhere in the test file to avoid bypassing type checks in visibility
assertions. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
This is a direct `any` annotation in a modified TypeScript test file, so it
violates the custom rule against using `any` in new or changed TS/TSX code.
</details>
[Fix in
Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=287f0bf684ba49938d07a3b0808b58cc&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=287f0bf684ba49938d07a3b0808b58cc&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/Scatter/controlPanel.test.ts
**Line:** 199:199
**Comment:**
*Custom Rule: Change this `any` usage to the same concrete control type
used elsewhere in the test file to avoid bypassing type checks in visibility
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=58291e70b650a86c9c76b46093943267b291a40189c0926dba161863cad1f061&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40967&comment_hash=58291e70b650a86c9c76b46093943267b291a40189c0926dba161863cad1f061&reaction=dislike'>👎</a>
##########
superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/Scatter/transformProps.test.ts:
##########
@@ -175,3 +175,283 @@ describe('Scatter Chart X-axis Number Formatting', () => {
},
);
});
+
+describe('Scatter Chart Orientation and Dot Size Metric', () => {
+ const baseFormData: EchartsTimeseriesFormData = {
+ ...DEFAULT_FORM_DATA,
+ colorScheme: 'supersetColors',
+ datasource: '1__table',
+ metrics: ['sum_val'],
+ x_axis: 'category_col',
+ groupby: [],
+ viz_type: 'echarts_timeseries_scatter',
+ seriesType: EchartsTimeseriesSeriesType.Scatter,
+ };
+
+ const categoricalData = [
+ {
+ data: [
+ { category_col: 'A', sum_val: 1, size_metric: 10 },
+ { category_col: 'B', sum_val: 2, size_metric: 25 },
+ { category_col: 'C', sum_val: 3, size_metric: 40 },
+ ],
+ colnames: ['category_col', 'sum_val', 'size_metric'],
+ coltypes: [
+ GenericDataType.String,
+ GenericDataType.Numeric,
+ GenericDataType.Numeric,
+ ],
+ label_map: {
+ category_col: ['category_col'],
+ sum_val: ['sum_val'],
+ size_metric: ['size_metric'],
+ },
+ },
+ ];
+
+ const baseChartPropsConfig = {
+ width: 800,
+ height: 600,
+ theme: supersetTheme,
+ };
+
+ const getScatterSeries = (props: ReturnType<typeof transformProps>) =>
+ (props.echartOptions.series as any[]).filter(s => s.type === 'scatter');
Review Comment:
**Suggestion:** Replace the `any[]` cast on the series collection with a
concrete ECharts series type so scatter filtering is type-safe. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
This is TypeScript test code and it uses `any[]` directly in a cast. The
custom rule explicitly flags any new or modified TypeScript/TSX code that uses
`any`, so this is a real violation.
</details>
[Fix in
Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=ae7feb05f8fa440e92527820d58dd0c8&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=ae7feb05f8fa440e92527820d58dd0c8&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/Scatter/transformProps.test.ts
**Line:** 218:219
**Comment:**
*Custom Rule: Replace the `any[]` cast on the series collection with a
concrete ECharts series type so scatter filtering is type-safe.
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=8be97440ad089f23634ec6575640b5352aa1727fe444ffa4350ddcca405c8710&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40967&comment_hash=8be97440ad089f23634ec6575640b5352aa1727fe444ffa4350ddcca405c8710&reaction=dislike'>👎</a>
##########
superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/Scatter/controlPanel.test.ts:
##########
@@ -148,3 +148,59 @@ test('x_axis_number_format control should be hidden for
non-numeric data types',
expect(isNumberVisible(null, null)).toBe(false);
expect(isNumberVisible('time_column', GenericDataType.Temporal)).toBe(false);
});
+
+// tests for orientation and dot size controls
+const orientationControl: any = getControl('orientation');
+const sizeControl: any = getControl('size');
+const minMarkerSizeControl: any = getControl('minMarkerSize');
+const maxMarkerSizeControl: any = getControl('maxMarkerSize');
Review Comment:
**Suggestion:** Replace these `any` annotations with a concrete control type
(or a shared reusable control interface from the chart controls types) so the
assertions on `config` and `visibility` are type-safe. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
The file is a TypeScript test file, and these newly added declarations use
the banned `any` type directly. This matches the custom rule requiring a
concrete, reusable, or narrower type instead of `any`.
</details>
[Fix in
Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=1357b71bf0644e18b35999d4fdd14711&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=1357b71bf0644e18b35999d4fdd14711&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/Scatter/controlPanel.test.ts
**Line:** 152:156
**Comment:**
*Custom Rule: Replace these `any` annotations with a concrete control
type (or a shared reusable control interface from the chart controls types) so
the assertions on `config` and `visibility` are type-safe.
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=bca55730d1805d4943eccfddec623a0d84e58155ed273c896ed4bd3da08c2898&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40967&comment_hash=bca55730d1805d4943eccfddec623a0d84e58155ed273c896ed4bd3da08c2898&reaction=dislike'>👎</a>
##########
superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/Scatter/transformProps.test.ts:
##########
@@ -175,3 +175,283 @@ describe('Scatter Chart X-axis Number Formatting', () => {
},
);
});
+
+describe('Scatter Chart Orientation and Dot Size Metric', () => {
+ const baseFormData: EchartsTimeseriesFormData = {
+ ...DEFAULT_FORM_DATA,
+ colorScheme: 'supersetColors',
+ datasource: '1__table',
+ metrics: ['sum_val'],
+ x_axis: 'category_col',
+ groupby: [],
+ viz_type: 'echarts_timeseries_scatter',
+ seriesType: EchartsTimeseriesSeriesType.Scatter,
+ };
+
+ const categoricalData = [
+ {
+ data: [
+ { category_col: 'A', sum_val: 1, size_metric: 10 },
+ { category_col: 'B', sum_val: 2, size_metric: 25 },
+ { category_col: 'C', sum_val: 3, size_metric: 40 },
+ ],
+ colnames: ['category_col', 'sum_val', 'size_metric'],
+ coltypes: [
+ GenericDataType.String,
+ GenericDataType.Numeric,
+ GenericDataType.Numeric,
+ ],
+ label_map: {
+ category_col: ['category_col'],
+ sum_val: ['sum_val'],
+ size_metric: ['size_metric'],
+ },
+ },
+ ];
+
+ const baseChartPropsConfig = {
+ width: 800,
+ height: 600,
+ theme: supersetTheme,
+ };
+
+ const getScatterSeries = (props: ReturnType<typeof transformProps>) =>
+ (props.echartOptions.series as any[]).filter(s => s.type === 'scatter');
+
+ const singleMetricData = [
+ {
+ ...categoricalData[0],
+ data: categoricalData[0].data.map(row => ({
+ category_col: row.category_col,
+ sum_val: row.sum_val,
+ })),
+ colnames: ['category_col', 'sum_val'],
+ coltypes: [GenericDataType.String, GenericDataType.Numeric],
+ },
+ ];
+
+ test('horizontal orientation swaps the dimension axis onto the y-axis', ()
=> {
+ const chartProps = new ChartProps({
+ ...baseChartPropsConfig,
+ queriesData: singleMetricData,
+ formData: { ...baseFormData, orientation: 'horizontal' },
+ });
+
+ const transformedProps = transformProps(
+ chartProps as EchartsTimeseriesChartProps,
+ );
+ const { xAxis, yAxis } = transformedProps.echartOptions as any;
Review Comment:
**Suggestion:** Avoid casting axis config to `any`; use the proper ECharts
axis option type (or a narrowed union) for the destructured axis objects.
[custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
The code casts the chart options object to `any`, which is explicitly
disallowed by the rule for TypeScript/TSX changes. This is a direct and present
violation.
</details>
[Fix in
Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=fd5a87534e5f4d5aaa37fad3a3cb57ad&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=fd5a87534e5f4d5aaa37fad3a3cb57ad&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/Scatter/transformProps.test.ts
**Line:** 243:243
**Comment:**
*Custom Rule: Avoid casting axis config to `any`; use the proper
ECharts axis option type (or a narrowed union) for the destructured axis
objects.
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=23ec214352420cb43514444aaa1b7dc7f05a03b2f4741d77ffab9e38464069e0&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40967&comment_hash=23ec214352420cb43514444aaa1b7dc7f05a03b2f4741d77ffab9e38464069e0&reaction=dislike'>👎</a>
##########
superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/Scatter/transformProps.test.ts:
##########
@@ -175,3 +175,283 @@ describe('Scatter Chart X-axis Number Formatting', () => {
},
);
});
+
+describe('Scatter Chart Orientation and Dot Size Metric', () => {
+ const baseFormData: EchartsTimeseriesFormData = {
+ ...DEFAULT_FORM_DATA,
+ colorScheme: 'supersetColors',
+ datasource: '1__table',
+ metrics: ['sum_val'],
+ x_axis: 'category_col',
+ groupby: [],
+ viz_type: 'echarts_timeseries_scatter',
+ seriesType: EchartsTimeseriesSeriesType.Scatter,
+ };
+
+ const categoricalData = [
+ {
+ data: [
+ { category_col: 'A', sum_val: 1, size_metric: 10 },
+ { category_col: 'B', sum_val: 2, size_metric: 25 },
+ { category_col: 'C', sum_val: 3, size_metric: 40 },
+ ],
+ colnames: ['category_col', 'sum_val', 'size_metric'],
+ coltypes: [
+ GenericDataType.String,
+ GenericDataType.Numeric,
+ GenericDataType.Numeric,
+ ],
+ label_map: {
+ category_col: ['category_col'],
+ sum_val: ['sum_val'],
+ size_metric: ['size_metric'],
+ },
+ },
+ ];
+
+ const baseChartPropsConfig = {
+ width: 800,
+ height: 600,
+ theme: supersetTheme,
+ };
+
+ const getScatterSeries = (props: ReturnType<typeof transformProps>) =>
+ (props.echartOptions.series as any[]).filter(s => s.type === 'scatter');
+
+ const singleMetricData = [
+ {
+ ...categoricalData[0],
+ data: categoricalData[0].data.map(row => ({
+ category_col: row.category_col,
+ sum_val: row.sum_val,
+ })),
+ colnames: ['category_col', 'sum_val'],
+ coltypes: [GenericDataType.String, GenericDataType.Numeric],
+ },
+ ];
+
+ test('horizontal orientation swaps the dimension axis onto the y-axis', ()
=> {
+ const chartProps = new ChartProps({
+ ...baseChartPropsConfig,
+ queriesData: singleMetricData,
+ formData: { ...baseFormData, orientation: 'horizontal' },
+ });
+
+ const transformedProps = transformProps(
+ chartProps as EchartsTimeseriesChartProps,
+ );
+ const { xAxis, yAxis } = transformedProps.echartOptions as any;
+ expect(yAxis.type).toBe('category');
+ expect(xAxis.type).toBe('value');
+
+ const series = getScatterSeries(transformedProps);
+ expect(series).toHaveLength(1);
+ // data points are flipped to [metric, dimension]
+ expect(series[0].data[0]).toEqual([1, 'A']);
+ });
+
+ test('vertical orientation keeps the dimension axis on the x-axis', () => {
+ const chartProps = new ChartProps({
+ ...baseChartPropsConfig,
+ queriesData: singleMetricData,
+ formData: baseFormData,
+ });
+
+ const transformedProps = transformProps(
+ chartProps as EchartsTimeseriesChartProps,
+ );
+ const { xAxis, yAxis } = transformedProps.echartOptions as any;
+ expect(xAxis.type).toBe('category');
+ expect(yAxis.type).toBe('value');
+
+ const series = getScatterSeries(transformedProps);
+ expect(series[0].data[0]).toEqual(['A', 1]);
+ });
+
+ test('size metric series is not rendered or shown in the legend', () => {
+ const chartProps = new ChartProps({
+ ...baseChartPropsConfig,
+ queriesData: categoricalData,
+ formData: { ...baseFormData, size: 'size_metric' },
+ });
+
+ const transformedProps = transformProps(
+ chartProps as EchartsTimeseriesChartProps,
+ );
+ const series = getScatterSeries(transformedProps);
+ expect(series).toHaveLength(1);
+ expect(series[0].name).toBe('sum_val');
+ expect(transformedProps.legendData).toEqual(['sum_val']);
+ });
+
+ test('size metric scales marker areas between min and max dot size', () => {
+ const chartProps = new ChartProps({
+ ...baseChartPropsConfig,
+ queriesData: categoricalData,
+ formData: {
+ ...baseFormData,
+ size: 'size_metric',
+ minMarkerSize: 5,
+ maxMarkerSize: 30,
+ },
+ });
+
+ const transformedProps = transformProps(
+ chartProps as EchartsTimeseriesChartProps,
+ );
+ const [series] = getScatterSeries(transformedProps);
+ expect(typeof series.symbolSize).toBe('function');
+ // smallest size value -> min dot size, largest -> max dot size
+ expect(series.symbolSize(['A', 1])).toBe(5);
+ expect(series.symbolSize(['C', 3])).toBe(30);
+ // midpoint value -> midpoint *area*, not midpoint diameter
+ expect(series.symbolSize(['B', 2]) ** 2).toBeCloseTo(
+ (5 ** 2 + 30 ** 2) / 2,
+ );
+ });
+
+ test('size metric lookups follow the dimension key when grouped', () => {
+ const groupedData = [
+ {
+ data: [
+ {
+ category_col: 'A',
+ 'sum_val, g1': 1,
+ 'size_metric, g1': 10,
+ 'sum_val, g2': 2,
+ 'size_metric, g2': 40,
+ },
+ ],
+ colnames: [
+ 'category_col',
+ 'sum_val, g1',
+ 'size_metric, g1',
+ 'sum_val, g2',
+ 'size_metric, g2',
+ ],
+ coltypes: [
+ GenericDataType.String,
+ GenericDataType.Numeric,
+ GenericDataType.Numeric,
+ GenericDataType.Numeric,
+ GenericDataType.Numeric,
+ ],
+ label_map: {
+ category_col: ['category_col'],
+ 'sum_val, g1': ['sum_val', 'g1'],
+ 'size_metric, g1': ['size_metric', 'g1'],
+ 'sum_val, g2': ['sum_val', 'g2'],
+ 'size_metric, g2': ['size_metric', 'g2'],
+ },
+ },
+ ];
+ const chartProps = new ChartProps({
+ ...baseChartPropsConfig,
+ queriesData: groupedData,
+ formData: {
+ ...baseFormData,
+ groupby: ['group_col'],
+ size: 'size_metric',
+ minMarkerSize: 5,
+ maxMarkerSize: 30,
+ },
+ });
+
+ const transformedProps = transformProps(
+ chartProps as EchartsTimeseriesChartProps,
+ );
+ const series = getScatterSeries(transformedProps);
+ expect(series.map((s: any) => s.name).sort()).toEqual([
+ 'sum_val, g1',
+ 'sum_val, g2',
+ ]);
+ const g1 = series.find((s: any) => s.name === 'sum_val, g1');
+ const g2 = series.find((s: any) => s.name === 'sum_val, g2');
Review Comment:
**Suggestion:** Type the `find` callback parameter with the same series type
used elsewhere rather than `any` to enforce safe property access. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
Both callback parameters are typed as `any` in TypeScript test code. That
directly violates the rule requiring concrete or narrower types instead of
`any`.
</details>
[Fix in
Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=78950423cd8446fd8b890fa01a58066e&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=78950423cd8446fd8b890fa01a58066e&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/Scatter/transformProps.test.ts
**Line:** 368:369
**Comment:**
*Custom Rule: Type the `find` callback parameter with the same series
type used elsewhere rather than `any` to enforce safe property access.
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=01d159edd6e7a94ec0a5c2854c7ccdf6cea56b5fc538aad2102493d1aa992393&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40967&comment_hash=01d159edd6e7a94ec0a5c2854c7ccdf6cea56b5fc538aad2102493d1aa992393&reaction=dislike'>👎</a>
##########
superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/Scatter/transformProps.test.ts:
##########
@@ -175,3 +175,283 @@ describe('Scatter Chart X-axis Number Formatting', () => {
},
);
});
+
+describe('Scatter Chart Orientation and Dot Size Metric', () => {
+ const baseFormData: EchartsTimeseriesFormData = {
+ ...DEFAULT_FORM_DATA,
+ colorScheme: 'supersetColors',
+ datasource: '1__table',
+ metrics: ['sum_val'],
+ x_axis: 'category_col',
+ groupby: [],
+ viz_type: 'echarts_timeseries_scatter',
+ seriesType: EchartsTimeseriesSeriesType.Scatter,
+ };
+
+ const categoricalData = [
+ {
+ data: [
+ { category_col: 'A', sum_val: 1, size_metric: 10 },
+ { category_col: 'B', sum_val: 2, size_metric: 25 },
+ { category_col: 'C', sum_val: 3, size_metric: 40 },
+ ],
+ colnames: ['category_col', 'sum_val', 'size_metric'],
+ coltypes: [
+ GenericDataType.String,
+ GenericDataType.Numeric,
+ GenericDataType.Numeric,
+ ],
+ label_map: {
+ category_col: ['category_col'],
+ sum_val: ['sum_val'],
+ size_metric: ['size_metric'],
+ },
+ },
+ ];
+
+ const baseChartPropsConfig = {
+ width: 800,
+ height: 600,
+ theme: supersetTheme,
+ };
+
+ const getScatterSeries = (props: ReturnType<typeof transformProps>) =>
+ (props.echartOptions.series as any[]).filter(s => s.type === 'scatter');
+
+ const singleMetricData = [
+ {
+ ...categoricalData[0],
+ data: categoricalData[0].data.map(row => ({
+ category_col: row.category_col,
+ sum_val: row.sum_val,
+ })),
+ colnames: ['category_col', 'sum_val'],
+ coltypes: [GenericDataType.String, GenericDataType.Numeric],
+ },
+ ];
+
+ test('horizontal orientation swaps the dimension axis onto the y-axis', ()
=> {
+ const chartProps = new ChartProps({
+ ...baseChartPropsConfig,
+ queriesData: singleMetricData,
+ formData: { ...baseFormData, orientation: 'horizontal' },
+ });
+
+ const transformedProps = transformProps(
+ chartProps as EchartsTimeseriesChartProps,
+ );
+ const { xAxis, yAxis } = transformedProps.echartOptions as any;
+ expect(yAxis.type).toBe('category');
+ expect(xAxis.type).toBe('value');
+
+ const series = getScatterSeries(transformedProps);
+ expect(series).toHaveLength(1);
+ // data points are flipped to [metric, dimension]
+ expect(series[0].data[0]).toEqual([1, 'A']);
+ });
+
+ test('vertical orientation keeps the dimension axis on the x-axis', () => {
+ const chartProps = new ChartProps({
+ ...baseChartPropsConfig,
+ queriesData: singleMetricData,
+ formData: baseFormData,
+ });
+
+ const transformedProps = transformProps(
+ chartProps as EchartsTimeseriesChartProps,
+ );
+ const { xAxis, yAxis } = transformedProps.echartOptions as any;
+ expect(xAxis.type).toBe('category');
+ expect(yAxis.type).toBe('value');
+
+ const series = getScatterSeries(transformedProps);
+ expect(series[0].data[0]).toEqual(['A', 1]);
+ });
+
+ test('size metric series is not rendered or shown in the legend', () => {
+ const chartProps = new ChartProps({
+ ...baseChartPropsConfig,
+ queriesData: categoricalData,
+ formData: { ...baseFormData, size: 'size_metric' },
+ });
+
+ const transformedProps = transformProps(
+ chartProps as EchartsTimeseriesChartProps,
+ );
+ const series = getScatterSeries(transformedProps);
+ expect(series).toHaveLength(1);
+ expect(series[0].name).toBe('sum_val');
+ expect(transformedProps.legendData).toEqual(['sum_val']);
+ });
+
+ test('size metric scales marker areas between min and max dot size', () => {
+ const chartProps = new ChartProps({
+ ...baseChartPropsConfig,
+ queriesData: categoricalData,
+ formData: {
+ ...baseFormData,
+ size: 'size_metric',
+ minMarkerSize: 5,
+ maxMarkerSize: 30,
+ },
+ });
+
+ const transformedProps = transformProps(
+ chartProps as EchartsTimeseriesChartProps,
+ );
+ const [series] = getScatterSeries(transformedProps);
+ expect(typeof series.symbolSize).toBe('function');
+ // smallest size value -> min dot size, largest -> max dot size
+ expect(series.symbolSize(['A', 1])).toBe(5);
+ expect(series.symbolSize(['C', 3])).toBe(30);
+ // midpoint value -> midpoint *area*, not midpoint diameter
+ expect(series.symbolSize(['B', 2]) ** 2).toBeCloseTo(
+ (5 ** 2 + 30 ** 2) / 2,
+ );
+ });
+
+ test('size metric lookups follow the dimension key when grouped', () => {
+ const groupedData = [
+ {
+ data: [
+ {
+ category_col: 'A',
+ 'sum_val, g1': 1,
+ 'size_metric, g1': 10,
+ 'sum_val, g2': 2,
+ 'size_metric, g2': 40,
+ },
+ ],
+ colnames: [
+ 'category_col',
+ 'sum_val, g1',
+ 'size_metric, g1',
+ 'sum_val, g2',
+ 'size_metric, g2',
+ ],
+ coltypes: [
+ GenericDataType.String,
+ GenericDataType.Numeric,
+ GenericDataType.Numeric,
+ GenericDataType.Numeric,
+ GenericDataType.Numeric,
+ ],
+ label_map: {
+ category_col: ['category_col'],
+ 'sum_val, g1': ['sum_val', 'g1'],
+ 'size_metric, g1': ['size_metric', 'g1'],
+ 'sum_val, g2': ['sum_val', 'g2'],
+ 'size_metric, g2': ['size_metric', 'g2'],
+ },
+ },
+ ];
+ const chartProps = new ChartProps({
+ ...baseChartPropsConfig,
+ queriesData: groupedData,
+ formData: {
+ ...baseFormData,
+ groupby: ['group_col'],
+ size: 'size_metric',
+ minMarkerSize: 5,
+ maxMarkerSize: 30,
+ },
+ });
+
+ const transformedProps = transformProps(
+ chartProps as EchartsTimeseriesChartProps,
+ );
+ const series = getScatterSeries(transformedProps);
+ expect(series.map((s: any) => s.name).sort()).toEqual([
Review Comment:
**Suggestion:** Replace the callback parameter type `any` with a typed
scatter series object so `.name` access is validated by TypeScript.
[custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
The callback parameter is explicitly typed as `any`, which falls under the
custom rule banning `any` in TypeScript/TSX code.
</details>
[Fix in
Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=86d471c2415e47db9c39ace481e8ba29&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=86d471c2415e47db9c39ace481e8ba29&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/Scatter/transformProps.test.ts
**Line:** 364:364
**Comment:**
*Custom Rule: Replace the callback parameter type `any` with a typed
scatter series object so `.name` access is validated by TypeScript.
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=8ddf532e349c77cc1700f3f39bc45bd4fda1f6d6de29c8d1ee7d45ada0d0e45a&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40967&comment_hash=8ddf532e349c77cc1700f3f39bc45bd4fda1f6d6de29c8d1ee7d45ada0d0e45a&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]
