codeant-ai-for-open-source[bot] commented on code in PR #34794:
URL: https://github.com/apache/superset/pull/34794#discussion_r3607467979
##########
superset-frontend/plugins/plugin-chart-echarts/src/MixedTimeseries/transformProps.ts:
##########
@@ -425,8 +433,17 @@ export default function transformProps(
const array = ensureIsArray(chartProps.rawFormData?.time_compare);
const inverted = invert(verboseMap);
+ let patternIncrement = 0;
rawSeriesA.forEach(entry => {
+ const derivedSeries = isDerivedSeries(entry, chartProps.rawFormData);
+ const lineStyle: LineStyleOption = {};
+ if (derivedSeries) {
+ patternIncrement += 1;
+ // use a combination of dash and dot for the line style
+ lineStyle.type = [(patternIncrement % 5) + 1, (patternIncrement % 3) +
1];
+ lineStyle.opacity = OpacityEnum.DerivedSeries;
Review Comment:
**Suggestion:** The new dashed styling is applied whenever a series is
derived, but regular timeseries behavior only applies this styling when
`timeShiftColor` is enabled. Without that gate, mixed charts will unexpectedly
render comparison series as dashed even when users did not opt into time-shift
styling. Add `timeShiftColor` to the condition before assigning dash/opacity
styles. [incorrect condition logic]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ MixedTimeseries ignores time_shift_color option for dash styling.
- ⚠️ Visual behavior diverges from echarts_timeseries_line expectations.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. The MixedTimeseries chart plugin registers `transformProps` as its
transform function
at
`superset-frontend/plugins/plugin-chart-echarts/src/MixedTimeseries/index.ts:23`
and
`:85`, so any `viz_type: 'mixed_timeseries'` chart will invoke
`transformProps` from
`superset-frontend/plugins/plugin-chart-echarts/src/MixedTimeseries/transformProps.ts:132`.
2. In `transformProps`, time comparison configuration from
`chartProps.rawFormData.time_compare` is read into `array` and `verboseMap`
is inverted at
`superset-frontend/plugins/plugin-chart-echarts/src/MixedTimeseries/transformProps.ts:435`,
and `rawSeriesA` / `rawSeriesB` contain both original and time-shifted
(derived) series
for the chart.
3. For each entry in `rawSeriesA`, derived detection is computed as `const
derivedSeries =
isDerivedSeries(entry, chartProps.rawFormData);` at `transformProps.ts:439`,
and a fresh
`lineStyle: LineStyleOption = {};` is created at `:440`; when
`derivedSeries` is true, the
block at `lines 441–445` unconditionally sets a dash pattern and opacity
(`lineStyle.type
= [...]` and `lineStyle.opacity = OpacityEnum.DerivedSeries;`) without
checking
`timeShiftColor`.
4. The same pattern is applied for `rawSeriesB` at
`transformProps.ts:520–526`, while
`timeShiftColor` is independently passed to `transformSeries` in the options
at `lines
507` and `591`; as a result, for any MixedTimeseries chart where time
comparison is
configured (derived series exist) but the `time_shift_color` control is left
disabled,
derived series will still render with dashed / semi-transparent styling,
diverging from
the regular timeseries implementation in
`superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts:18–30`
where dashed styling is gated by `if (derivedSeries && timeShiftColor)`.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=71bdae1d54244ad08bf40971316ad332&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=71bdae1d54244ad08bf40971316ad332&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/MixedTimeseries/transformProps.ts
**Line:** 441:445
**Comment:**
*Incorrect Condition Logic: The new dashed styling is applied whenever
a series is derived, but regular timeseries behavior only applies this styling
when `timeShiftColor` is enabled. Without that gate, mixed charts will
unexpectedly render comparison series as dashed even when users did not opt
into time-shift styling. Add `timeShiftColor` to the condition before assigning
dash/opacity styles.
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%2F34794&comment_hash=c45f72ca42bc94fa355050ca51d2f6047209c7cfca92c9640464e57350c8cf86&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F34794&comment_hash=c45f72ca42bc94fa355050ca51d2f6047209c7cfca92c9640464e57350c8cf86&reaction=dislike'>👎</a>
##########
superset-frontend/plugins/plugin-chart-echarts/src/MixedTimeseries/transformProps.ts:
##########
@@ -425,8 +433,17 @@ export default function transformProps(
const array = ensureIsArray(chartProps.rawFormData?.time_compare);
const inverted = invert(verboseMap);
+ let patternIncrement = 0;
Review Comment:
**Suggestion:** The dash-pattern assignment increments per series instead of
per time offset, so series representing the same offset can get different
patterns across dimensions/queries. This diverges from the established
timeseries behavior where one offset maps to one stable pattern, and it makes
comparison semantics inconsistent. Derive the pattern key from the actual
offset value rather than a global increment counter. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Same offset series get different dash patterns inconsistently.
- ⚠️ Comparison readability reduced in multi-dimension mixed charts.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. In MixedTimeseries `transformProps`, a single counter `let
patternIncrement = 0;` is
declared at
`superset-frontend/plugins/plugin-chart-echarts/src/MixedTimeseries/transformProps.ts:436`
just before iterating over `rawSeriesA` and `rawSeriesB`.
2. For each derived series in `rawSeriesA`, `patternIncrement` is
incremented and used
directly to compute the dash pattern at `transformProps.ts:441–445`
(`patternIncrement +=
1; lineStyle.type = [(patternIncrement % 5) + 1, (patternIncrement % 3) +
1];`), and the
same shared counter is reused in the `rawSeriesB.forEach` loop at `lines
520–526`.
3. This means the dash pattern index is determined solely by the order in
which derived
series are visited across both queries; if multiple derived series represent
the same time
offset (e.g., `"28 days ago, boy"` and `"28 days ago, girl"` in a grouped
MixedTimeseries
chart), they will receive different `patternIncrement` values depending on
iteration
order, and therefore different dash patterns even though they share the same
comparison
window.
4. In contrast, the regular timeseries transform at
`superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts:5–40`
maintains an `offsetLineWidths` map keyed by the actual time offset string,
assigns
`patternIndex` based on that offset, and then computes `lineStyle.type` and
symbols from
the offset-specific `patternIndex`, ensuring that all series for a given
offset share a
stable, consistent dash pattern; MixedTimeseries’s global counter breaks
this invariant
and yields inconsistent dash semantics for the same offset.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=6abd3fe3005345f6938331f23b99a5a6&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=6abd3fe3005345f6938331f23b99a5a6&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/MixedTimeseries/transformProps.ts
**Line:** 436:436
**Comment:**
*Logic Error: The dash-pattern assignment increments per series instead
of per time offset, so series representing the same offset can get different
patterns across dimensions/queries. This diverges from the established
timeseries behavior where one offset maps to one stable pattern, and it makes
comparison semantics inconsistent. Derive the pattern key from the actual
offset value rather than a global increment counter.
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%2F34794&comment_hash=f88ec1c142d7cf1451e7df0e2fa828c940b1ad43a04dc2342ce2c07e51998e33&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F34794&comment_hash=f88ec1c142d7cf1451e7df0e2fa828c940b1ad43a04dc2342ce2c07e51998e33&reaction=dislike'>👎</a>
##########
superset-frontend/plugins/plugin-chart-echarts/src/MixedTimeseries/transformProps.ts:
##########
@@ -425,8 +433,17 @@ export default function transformProps(
const array = ensureIsArray(chartProps.rawFormData?.time_compare);
const inverted = invert(verboseMap);
+ let patternIncrement = 0;
rawSeriesA.forEach(entry => {
+ const derivedSeries = isDerivedSeries(entry, chartProps.rawFormData);
Review Comment:
**Suggestion:** `isDerivedSeries` is called without the optional
`seriesName` argument, which is required for exact-offset names (for example a
series named exactly like `1 week ago`). In those cases the helper returns
false and derived comparison lines are not recognized, so dashed styling is
skipped. Pass the resolved series name as the third argument, as done in the
regular timeseries transform. [api mismatch]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Some derived comparison lines never detected, remain solid lines.
- ⚠️ Time comparison legend inconsistent for offset-only labels.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. In `transformProps` for MixedTimeseries at
`superset-frontend/plugins/plugin-chart-echarts/src/MixedTimeseries/transformProps.ts:39–65`,
each `rawSeriesA` entry’s human-readable name is resolved from `verboseMap`
as `const
entryName = String(entry.name || ''); const seriesName = inverted[entryName]
||
entryName;`, then used for labels and color scale keys.
2. Immediately after, derived-series detection is performed via `const
derivedSeries =
isDerivedSeries(entry, chartProps.rawFormData);` at `transformProps.ts:439`,
passing only
the `entry` and `rawFormData` arguments and omitting the resolved
`seriesName`.
3. In the regular timeseries transform,
`superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts:18–26`
documents and uses `isDerivedSeries(entry, chartProps.rawFormData,
seriesName)`,
explicitly noting that the helper checks three patterns, including an exact
match pattern
where `seriesName` equals a time offset such as `"1 day ago"` (comment at
`Timeseries/transformProps.ts:18–21`).
4. For MixedTimeseries configurations matching the documented “exact match”
case (single
metric, no dimensions, where `seriesName` is exactly a time offset as
described at
`Timeseries/transformProps.ts:77–79`), `isDerivedSeries` relies on the
`seriesName`
parameter to classify the series as derived; because MixedTimeseries omits
this third
argument at `transformProps.ts:439` and similarly at `:520`, `derivedSeries`
stays false
for those offset-only labels, so the derived comparison lines are not
recognized and never
receive the intended derived-specific styling (dash pattern and opacity
block at `lines
441–445` and `522–526`).
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=026e2a73ee894cba9503b3233a5f4645&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=026e2a73ee894cba9503b3233a5f4645&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/MixedTimeseries/transformProps.ts
**Line:** 439:439
**Comment:**
*Api Mismatch: `isDerivedSeries` is called without the optional
`seriesName` argument, which is required for exact-offset names (for example a
series named exactly like `1 week ago`). In those cases the helper returns
false and derived comparison lines are not recognized, so dashed styling is
skipped. Pass the resolved series name as the third argument, as done in the
regular timeseries transform.
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%2F34794&comment_hash=766975f763f19ed9dbf53bad68510d2c42b53de76a51148bed0ef7f44ee1aaa6&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F34794&comment_hash=766975f763f19ed9dbf53bad68510d2c42b53de76a51148bed0ef7f44ee1aaa6&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]
