codeant-ai-for-open-source[bot] commented on code in PR #41933:
URL: https://github.com/apache/superset/pull/41933#discussion_r3559495034
##########
superset-frontend/plugins/plugin-chart-echarts/src/MixedTimeseries/transformProps.ts:
##########
@@ -810,14 +835,20 @@ export default function transformProps(
.forEach(key => {
const value = forecastValues[key];
// if there are no dimensions, key is a verbose name of a metric,
- // otherwise it is a comma separated string where the first part
is metric name
+ // otherwise it is a comma separated string where the first part
is metric name.
+ // The tooltip key is the rendered series name, so resolve it
through
+ // the display-keyed maps rather than the raw backend-keyed ones.
let formatterKey;
if (primarySeries.has(key)) {
formatterKey =
- groupby.length === 0 ? inverted[key] : labelMap[key]?.[0];
+ groupby.length === 0
+ ? inverted[key]
+ : displayLabelMap[key]?.[0];
} else {
formatterKey =
- groupbyB.length === 0 ? inverted[key] : labelMapB[key]?.[0];
+ groupbyB.length === 0
+ ? inverted[key]
+ : displayLabelMapB[key]?.[0];
Review Comment:
**Suggestion:** For no-groupby charts, resolving tooltip formatter keys
through `inverted[key]` fails when rendered names include query suffixes like
`(Query A/B)`, so formatter lookup misses and falls back to generic formatting.
Resolve through the display-keyed label maps in this path as well so
query-identifier series still map to the correct raw metric key. [incorrect
variable usage]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
⚠️ Mixed Chart tooltips ignore metric-specific formatting.
⚠️ Query-identifier series show inconsistent numeric formatting.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. The Mixed Chart plugin uses `transformProps` from
`superset-frontend/plugins/plugin-chart-echarts/src/MixedTimeseries/transformProps.ts`
as
wired in `MixedTimeseries/index.ts:21-24,48-39`. `transformProps` returns
both the ECharts
options and display-keyed label maps (`displayLabelMap`, `displayLabelMapB`)
along with
`formData` that includes `groupby`/`groupbyB` (lines 445-454, 520-532,
534-612, and 20-31
of transformProps.ts).
2. For charts without groupby (i.e., `groupby` and `groupbyB` are empty
arrays; defaulted
in `DEFAULT_FORM_DATA` at `types.ts:134-135`), `transformProps` builds
`displayLabelMap`
by mapping the rendered series name (`displayName`) to the backend
`label_map` values
(transformProps.ts:455-478 for Query A, 534-557 for Query B). When
`showQueryIdentifiers`
is true (form option, default false at `types.ts:138`), `displayName` for
Query A metrics
is `${MetricDisplayNameA} (Query A)` and similarly for Query B
(transformProps.ts:462-473,
541-552).
3. The tooltip formatter in `transformProps` is configured in the
`tooltip.formatter`
function (transformProps.ts:810-879). It iterates `sortedKeys`, where each
`key` is the
rendered series name used as the legend label (transformProps.ts:819-835),
so for
no-groupby charts with query identifiers, `key` will be values like
`"sum__num (Query A)"`
or `"sum__num (Query B)"`.
4. In the block at `transformProps.ts:842-851`, the code chooses a
`formatterKey` for both
primary and secondary series: when `groupby.length === 0` it uses
`formatterKey =
inverted[key]` (and similarly for `groupbyB.length === 0`), where `inverted`
is
`invert(verboseMap)` (line 443). Because `key` includes the `(Query A)` or
`(Query B)`
suffix, it does not match any entry in `verboseMap`, so `inverted[key]` is
`undefined`. As
a result, `getFormatter(customFormatters, formatter, metrics, formatterKey,
...)` (lines
853-866) cannot resolve a metric-specific custom formatter and falls back to
the generic
axis formatter, causing tooltips for those query-identifier series to ignore
configured
per-metric formatting (for example, currency or percentage formatting) even
though
`displayLabelMap` already contains the correct backend label to use as a key.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=6168a5da3014430cb3cc79e9bee574d5&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=6168a5da3014430cb3cc79e9bee574d5&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:** 842:851
**Comment:**
*Incorrect Variable Usage: For no-groupby charts, resolving tooltip
formatter keys through `inverted[key]` fails when rendered names include query
suffixes like `(Query A/B)`, so formatter lookup misses and falls back to
generic formatting. Resolve through the display-keyed label maps in this path
as well so query-identifier series still map to the correct raw metric key.
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%2F41933&comment_hash=cca3dfe2087667b0e6e10a2212d93bfa598e76d378d1035b3cf5aa787c348199&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41933&comment_hash=cca3dfe2087667b0e6e10a2212d93bfa598e76d378d1035b3cf5aa787c348199&reaction=dislike'>👎</a>
##########
superset-frontend/plugins/plugin-chart-echarts/src/MixedTimeseries/EchartsMixedTimeseries.tsx:
##########
@@ -164,31 +169,35 @@ export default function EchartsMixedTimeseries({
formattedVal: xValueFormatter(data[0]),
});
}
- [
- ...(data && xAxis.type === AxisType.Category ? [xAxis.label] : []),
- ...(isFirst ? formData.groupby : formData.groupbyB),
- ].forEach((dimension, i) =>
+ if (data && xAxis.type === AxisType.Category && eventParams.name) {
drillToDetailFilters.push({
- col: dimension,
+ col: xAxis.label,
op: '==',
- val: values[i],
- formattedVal: String(values[i]),
- }),
- );
-
- [...(isFirst ? formData.groupby : formData.groupbyB)].forEach(
- (dimension, i) =>
+ val: eventParams.name,
+ formattedVal: eventParams.name,
+ });
Review Comment:
**Suggestion:** The category-axis drill-to-detail filter is gated by a
truthy check on `eventParams.name`, so valid category values like numeric `0`
are skipped and no x-axis filter is emitted. Check for `null`/`undefined`
explicitly instead of truthiness so zero-valued categories are preserved.
[falsy zero check]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
⚠️ Drill-to-detail on category zero omits x-axis filter.
⚠️ Mixed Chart drill behavior inconsistent across category values.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. The Mixed Chart plugin is registered in
`superset-frontend/plugins/plugin-chart-echarts/src/MixedTimeseries/index.ts:34-39`,
where
`transformProps` and `loadChart: () => import('./EchartsMixedTimeseries')`
are passed into
`EchartsChartPlugin`. This plugin advertises `Behavior.DrillToDetail` and
`Behavior.DrillBy`, so the `contextmenu` handler in `EchartsMixedTimeseries`
is used for
right-click drill actions.
2. `transformProps` computes `xAxisType` via `getAxisType`
(transformProps.ts:265-273) and
returns it as `xAxis.type` inside the transformed props object
(transformProps.ts:20-42).
When the chart is configured with a non-temporal x-axis (for example, a
numeric
dimension), `xAxis.type` can be `AxisType.Category`.
3. In the `EchartsMixedTimeseries` component’s `eventHandlers.contextmenu`
implementation
at
`superset-frontend/plugins/plugin-chart-echarts/src/MixedTimeseries/EchartsMixedTimeseries.tsx:147-215`,
the following block handles drill-to-detail on category axes: `if (data &&
xAxis.type ===
AxisType.Category && eventParams.name) { drillToDetailFilters.push({ col:
xAxis.label, op:
'==', val: eventParams.name, formattedVal: eventParams.name }); }`.
4. According to ECharts’ event model (used via `Echart` wrapper),
`eventParams.name` for a
category axis is the category value and is typed as `string | number`. When
the category
value is the number `0`, `eventParams.name` is a falsy value in JavaScript,
causing the
condition `... && eventParams.name` to fail. In that case the x-axis drill
filter is not
pushed into `drillToDetailFilters`, so right-clicking the 0 category
produces a
drill-to-detail request missing the x-axis filter, while other nonzero
categories do add
the filter. This manifests as drill-to-detail queries including all x-axis
values instead
of just the selected 0 category.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=bd980313b315408ca9b4bb1f6d00d3d4&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=bd980313b315408ca9b4bb1f6d00d3d4&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/EchartsMixedTimeseries.tsx
**Line:** 172:178
**Comment:**
*Falsy Zero Check: The category-axis drill-to-detail filter is gated by
a truthy check on `eventParams.name`, so valid category values like numeric `0`
are skipped and no x-axis filter is emitted. Check for `null`/`undefined`
explicitly instead of truthiness so zero-valued categories are preserved.
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%2F41933&comment_hash=85c9e93461e768ab6dbffcf848f21b6ea54b16191ff4b044ec819592475e2461&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41933&comment_hash=85c9e93461e768ab6dbffcf848f21b6ea54b16191ff4b044ec819592475e2461&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]
