codeant-ai-for-open-source[bot] commented on code in PR #37229:
URL: https://github.com/apache/superset/pull/37229#discussion_r2702162587
##########
superset-frontend/plugins/plugin-chart-echarts/src/MixedTimeseries/transformProps.ts:
##########
@@ -135,14 +136,21 @@ export default function transformProps(
let focusedSeries: string | null = null;
const {
- verboseMap = {},
+ verboseMap: originalVerboseMap = {},
currencyFormats = {},
columnFormats = {},
} = datasource;
const { label_map: labelMap } =
queriesData[0] as TimeseriesChartDataResponseResult;
const { label_map: labelMapB } =
queriesData[1] as TimeseriesChartDataResponseResult;
+
+ const verboseMapWithLabelMap = addLabelMapToVerboseMap(
+ labelMap,
+ originalVerboseMap,
+ );
+ const verboseMap = addLabelMapToVerboseMap(labelMapB,
verboseMapWithLabelMap);
Review Comment:
**Suggestion:** Passing `labelMap`/`labelMapB` directly into
`addLabelMapToVerboseMap` can throw if either `label_map` is undefined
(Object.entries in the helper will receive undefined). Use a safe fallback
(empty object) when calling the helper so the code won't crash when a query
result omits `label_map`. [null pointer]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ MixedTimeseries charts crash at render when label_map missing.
- ⚠️ Tooltips fail to display metric labels correctly.
- ⚠️ UI shows blank chart area instead of visualization.
```
</details>
```suggestion
labelMap || {},
originalVerboseMap,
);
const verboseMap = addLabelMapToVerboseMap(labelMapB || {},
verboseMapWithLabelMap);
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Render a MixedTimeseries chart that uses transformProps: open the chart
which triggers
transformProps in
superset-frontend/plugins/plugin-chart-echarts/src/MixedTimeseries/transformProps.ts
(function export default transformProps). The local assignment of label
maps occurs at
lines 143-146 where labelMap / labelMapB are read from queriesData:
"const { label_map: labelMap } = queriesData[0] as
TimeseriesChartDataResponseResult;"
and
"const { label_map: labelMapB } = queriesData[1] as
TimeseriesChartDataResponseResult;".
2. Provide query results where one of the query results omits label_map
(i.e.,
queriesData[0].label_map === undefined).
This is realistic when the backend response doesn't include a label_map
for a given
query.
3. transformProps then calls addLabelMapToVerboseMap at lines 148-152
("const verboseMapWithLabelMap = addLabelMapToVerboseMap(labelMap,
originalVerboseMap);"),
passing the undefined labelMap into the helper.
4. Inside the helper at
superset-frontend/plugins/plugin-chart-echarts/src/utils/forecast.ts
(addLabelMapToVerboseMap, lines ~36-62),
the implementation does Object.entries(label_map). If label_map is
undefined this
throws a TypeError
("Cannot convert undefined or null to object"), causing transformProps to
crash and the
chart to fail rendering.
```
</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/MixedTimeseries/transformProps.ts
**Line:** 149:152
**Comment:**
*Null Pointer: Passing `labelMap`/`labelMapB` directly into
`addLabelMapToVerboseMap` can throw if either `label_map` is undefined
(Object.entries in the helper will receive undefined). Use a safe fallback
(empty object) when calling the helper so the code won't crash when a query
result omits `label_map`.
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/src/utils/forecast.ts:
##########
@@ -26,6 +26,41 @@ import {
} from '../types';
import { sanitizeHtml } from './series';
+/**
+ * Enriches the verbose map by creating human-readable versions of compound
field names.
+ *
+ * @param label_map — a mapping of compound keys to arrays of component labels
(e.g., { "revenue_total_usd": ["revenue", "total", "usd"] })
+ * @param verboseMap — the existing mapping of field names to their display
labels
+ * @returns an updated verbose map that includes human-readable versions of
the compound keys
+ */
+export const addLabelMapToVerboseMap = (
+ label_map: Record<string, string[]>,
+ verboseMap: Record<string, string> = {},
+): Record<string, string> => {
+ /**
+ * Logic:
+ * 1. Iterates through each entry in label_map
+ * 2. For each compound key and its component labels, filters to only labels
that have verbose mappings
+ * 3. Replaces each component label in the key with its verbose name from
verboseMap
+ * 4. Stores the transformed key in a new map
+ * 5. Returns the original verboseMap merged with the new transformed entries
+ */
+ const newVerboseMap: Record<string, string> = {};
+
+ Object.entries(label_map).forEach(([key, labels]) => {
+ if (labels) {
+ labels
+ .filter(l => verboseMap[l])
+ .forEach(l => {
+ const newKey = key.replaceAll(l, verboseMap[l]);
+ newVerboseMap[key] = newKey;
+ });
Review Comment:
**Suggestion:** Logic bug: when a compound key has multiple component labels
the current code replaces each label against the original `key` and writes
`newVerboseMap[key]` inside the inner loop, so only the last processed label
replacement is preserved; this causes partially transformed labels (missing
earlier replacements). Apply the replacements sequentially to a working copy of
the key and only store the fully transformed result once. [logic error]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ Forecast tooltip shows partially transformed metric labels.
- ⚠️ Chart UX displays confusing series names.
- ⚠️ Affects functions that consume enriched verbose maps.
```
</details>
```suggestion
const eligible = labels.filter(l => verboseMap[l]);
if (eligible.length) {
let transformedKey = key;
eligible.forEach(l => {
transformedKey = transformedKey.split(l).join(verboseMap[l]);
});
newVerboseMap[key] = transformedKey;
}
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open superset-frontend/plugins/plugin-chart-echarts/src/utils/forecast.ts
and find
addLabelMapToVerboseMap (hunk lines ~36-62). The inner loop that performs
replacements
runs at lines 52-56 and assigns newVerboseMap[key] repeatedly.
2. Write a small unit test or run in REPL:
const label_map = { 'revenue_total_usd': ['revenue','total','usd'] };
const verboseMap = { revenue: 'Revenue', total: 'Total', usd: 'USD' };
const out = addLabelMapToVerboseMap(label_map, verboseMap);
3. Observe the returned mapping newVerboseMap entry for key
'revenue_total_usd' — because
each replacement is applied against the original key and newVerboseMap[key]
is overwritten
on each iteration, only the replacement from the last iterated label (likely
'usd') is
preserved rather than a fully transformed 'RevenueTotalUSD'. This
demonstrates the logic
bug at forecast.ts lines 53-56.
4. The incorrect partial transformation means downstream code consuming
verboseMap (e.g.,
chart tooltip label resolution and rebaseForecastDatum usage) will show
incomplete or
incorrect human-readable names for multi-component metric keys.
```
</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/utils/forecast.ts
**Line:** 52:57
**Comment:**
*Logic Error: Logic bug: when a compound key has multiple component
labels the current code replaces each label against the original `key` and
writes `newVerboseMap[key]` inside the inner loop, so only the last processed
label replacement is preserved; this causes partially transformed labels
(missing earlier replacements). Apply the replacements sequentially to a
working copy of the key and only store the fully transformed result once.
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]
