codeant-ai-for-open-source[bot] commented on code in PR #37405:
URL: https://github.com/apache/superset/pull/37405#discussion_r2746596987
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformers.ts:
##########
@@ -361,8 +375,11 @@ export function transformSeries(
symbolSize: markerSize,
label: {
show: !!showValue,
- position: isHorizontal ? 'right' : 'top',
- color: theme?.colorText,
+ position: stack ? 'inside' : isHorizontal ? 'right' : 'top',
+ color:
+ stack || seriesType === 'bar'
+ ? getContrastingColor(String(itemStyle.color))
Review Comment:
**Suggestion:** The series-level `label.position` is set to a generic
`'inside'` when `stack` is true, which will override per-data label positions
(such as 'insideLeft'/'insideRight') created by
`transformNegativeLabelsPosition`. Also the `color` expression calls
`getContrastingColor(String(itemStyle.color))` without guarding against
`itemStyle.color` being undefined or a non-string, which can produce invalid
inputs. Change the series-level `position` to allow data-level overrides when
stacked and guard/resolves the item color before calling `getContrastingColor`,
falling back to `theme?.colorText`. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Stacked bar negative labels mispositioned visually.
- ⚠️ Timeseries ECharts rendering for stacked bars affected.
- ⚠️ Accessibility/readability of negative values impaired.
- ⚠️ Label color may be unexpected if color missing.
```
</details>
```suggestion
// Allow data-level label.position (e.g. 'insideLeft'/'insideRight')
to take precedence when stacked
position: stack ? undefined : isHorizontal ? 'right' : 'top',
color:
stack || seriesType === 'bar'
? (itemStyle.color ? getContrastingColor(String(itemStyle.color))
: theme?.colorText)
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Produce a stacked bar timeseries with negative values in Superset
(Timeseries ECharts
pipeline uses this file). The per-data transform is defined at
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformers.ts in
transformNegativeLabelsPosition (lines ~141-174 in the PR hunk). That
function returns
per-data label objects with position 'insideLeft' or 'insideRight' for
negative/positive
values.
2. When the chart renderer builds series options it calls transformSeries in
the same file
(series construction begins in the hunk around line 329). For a bar series
with stacking
enabled, transformSeries will call transformNegativeLabelsPosition(series,
isHorizontal)
to produce data entries with label positions (see
transformNegativeLabelsPosition at lines
~141-174).
3. Later in transformSeries the series-level label configuration is set to
position: stack
? 'inside' : ... (exact lines 376-378). ECharts applies a series-level
label.position over
data-level label.position, so this assignment overrides the per-data
positions produced in
step 2.
4. Render the chart in the browser and inspect the generated ECharts option
(developer
tools). You will observe that data-level label positions
('insideLeft'/'insideRight') are
not taking effect and all labels use the series-level 'inside' placement,
causing
negative-segment labels to be mispositioned relative to their bar edges
(visual regression
the PR intended to fix).
5. (Color fallback observation, lower-likelihood) The series-level color
expression calls
getContrastingColor(String(itemStyle.color)) at lines 379-381. If
itemStyle.color is
absent or not a plain color string (for example in a contrived test where
colorScale
returns undefined or a non-string object), String(itemStyle.color) yields
'undefined' or
'[object Object]'. This produces an unexpected input to getContrastingColor
and may yield
an invalid contrast color. Reproducing this requires forcing itemStyle.color
to be
undefined (e.g., mocking colorScale to return undefined) and observing label
color output.
This is a realistic but uncommon scenario.
Note: The label-position problem is a behavior vs. intention mismatch —
transformNegativeLabelsPosition sets data-level positions, but the later
series-level
position unconditionally overrides them.
```
</details>
<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/transformers.ts
**Line:** 378:381
**Comment:**
*Logic Error: The series-level `label.position` is set to a generic
`'inside'` when `stack` is true, which will override per-data label positions
(such as 'insideLeft'/'insideRight') created by
`transformNegativeLabelsPosition`. Also the `color` expression calls
`getContrastingColor(String(itemStyle.color))` without guarding against
`itemStyle.color` being undefined or a non-string, which can produce invalid
inputs. Change the series-level `position` to allow data-level overrides when
stacked and guard/resolves the item color before calling `getContrastingColor`,
falling back to `theme?.colorText`.
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/plugins/plugin-chart-echarts/test/Timeseries/transformers.test.ts:
##########
@@ -28,10 +28,12 @@ import {
import transformProps from '../../src/Timeseries/transformProps';
import { EchartsTimeseriesChartProps } from '../../src/types';
-// Mock the colorScale function
-const mockColorScale = jest.fn(
- (key: string, sliceId?: number) => `color-for-${key}-${sliceId}`,
-) as unknown as CategoricalColorScale;
+// Mock the colorScale function to return different colors based on key
+const mockColorScale = jest.fn((key: string) => {
+ if (key === 'test-key') return '#1f77b4'; // blue
+ if (key === 'series-key') return '#ff7f0e'; // orange
+ return '#2ca02c'; // green for any other key
+}) as unknown as CategoricalColorScale;
Review Comment:
**Suggestion:** The test's `mockColorScale` Jest mock is declared to accept
a single parameter but the production code calls the color scale with two
arguments (key and namespace/sliceId). Typing the mock to accept the optional
second parameter matches actual usage and avoids type/intent confusion; replace
the loose "unknown as" cast with a direct cast to `CategoricalColorScale`.
[type error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Unit test type-safety masked by double-cast.
- ⚠️ Developers may be misled about mock signature.
```
</details>
```suggestion
const mockColorScale = jest.fn((key: string, namespace?: number) => {
if (key === 'test-key') return '#1f77b4'; // blue
if (key === 'series-key') return '#ff7f0e'; // orange
return '#2ca02c'; // green for any other key
}) as CategoricalColorScale;
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open the unit test file at
superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/transformers.test.ts
and
locate the mock definition at lines 31-36. The mock is declared as:
// Mock the colorScale function to return different colors based on key
const mockColorScale = jest.fn((key: string) => { ... }) as unknown as
CategoricalColorScale;
2. Observe transformSeries in
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformers.ts
(function
starting at line 179). Inside that function the production code calls the
colorScale with
two arguments for itemStyle.color:
color: timeShiftColor ? colorScale(colorScaleKey, sliceId) :
colorScale(seriesKey ||
forecastSeries.name, sliceId)
3. The test has assertions that expect two-argument calls (e.g.
expect(mockColorScale).toHaveBeenCalledWith('test-key', 1) added at lines
~50-51 and
~63-64 in transformers.test.ts). At runtime Jest accepts any arguments and
the test will
pass because the mock implementation ignores typings.
4. If you remove the current double-cast hack ("as unknown as
CategoricalColorScale") and
instead cast the single-argument mock directly to CategoricalColorScale
without adding the
optional second parameter, TypeScript will raise a compilation/type error
when running the
test typecheck (yarn test / ts-jest or tsc). The concrete reproduction is:
- Change the mock cast to "as CategoricalColorScale" but keep the
single-arg function
(file: transformers.test.ts, line 36).
- Run the TypeScript compile step used by tests (jest with ts-jest or
tsc).
- Observe a type error complaining that "(key: string) => string" is not
assignable to
CategoricalColorScale because the second (namespace/sliceId) parameter is
missing.
```
</details>
<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/transformers.test.ts
**Line:** 32:36
**Comment:**
*Type Error: The test's `mockColorScale` Jest mock is declared to
accept a single parameter but the production code calls the color scale with
two arguments (key and namespace/sliceId). Typing the mock to accept the
optional second parameter matches actual usage and avoids type/intent
confusion; replace the loose "unknown as" cast with a direct cast to
`CategoricalColorScale`.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
```
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
