codeant-ai-for-open-source[bot] commented on code in PR #37229:
URL: https://github.com/apache/superset/pull/37229#discussion_r3454586355
##########
superset-frontend/plugins/plugin-chart-echarts/src/utils/forecast.ts:
##########
@@ -26,6 +26,147 @@ import {
} from '../types';
import { sanitizeHtml } from './series';
+/**
+ * Escapes RegExp metacharacters in a string so it can be safely used in a
+ * dynamically created regular expression.
+ *
+ * @param value - The raw string to escape
+ * @returns The escaped string safe for use in `new RegExp(...)`
+ */
+const escapeRegex = (value: string) =>
+ value.replace(/[.*+?^${}()|[\]\\]/g, '\\$&');
+
+/**
+ * Replaces a label inside a compound key only if it appears as a complete
+ * word/token and contains at least one alphabetic character.
+ *
+ * @param key - The source string (typically a compound field name)
+ * @param label - The label to search for as a standalone token
+ * @param replacement - The human-readable value to replace the label with
+ * @returns The transformed key if a valid match exists, otherwise the
original key
+ */
+const replaceLabelIfExists = (
+ key: string,
+ label: string,
+ replacement: string,
+) => {
+ /**
+ * Logic:
+ *
+ * This function is intentionally stricter than a simple substring replace:
+ * - The label must NOT be part of a larger word (e.g. "account" will NOT
match
+ * "testing_account").
+ * - Underscores (`_`) are treated as part of the word.
+ * - Numeric-only matches are ignored (e.g. "12" will NOT match "123").
+ *
+ * If the label is found, only the matched portion is replaced; otherwise,
+ * the original key is returned unchanged.
+ *
+ * Examples:
+ * - replaceLabelIfExists("testing_account 123", "testing_account",
"Account")
+ * → "Account 123"
+ * - replaceLabelIfExists("testing_account 123", "account", "Account")
+ * → "testing_account 123"
+ * - replaceLabelIfExists("123", "12", "X")
+ * → "123"
+ */
+
+ if (key === label) {
+ return replacement;
+ }
+
+ const escapedLabel = escapeRegex(label);
+ const regex = new RegExp(`(?<!\\w)${escapedLabel}(?!\\w)`, 'g');
+ return regex.test(key) ? key.replace(regex, replacement) : key;
+};
+
+/**
+ * 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:
+ *
+ * This function takes a mapping of compound field names to their component
labels
+ * and replaces those labels with their corresponding human-readable values
from
+ * `verboseMap`, producing display-friendly versions of the compound keys.
+ *
+ * Replacement behavior:
+ * - Each compound key is processed word-by-word (split on spaces).
+ * - Only labels that exist in `verboseMap` are considered.
+ * - Each word is replaced at most once, using `replaceLabelIfExists`, which:
+ * - Matches only full tokens (no partial matches).
+ * - Treats underscores (`_`) as part of a token.
+ * - Is case-sensitive.
+ * - Labels not found in `verboseMap` are left unchanged.
+ *
+ * The original `verboseMap` is preserved and extended with the newly
generated
+ * human-readable entries.
+ *
+ * Example:
+ * ```ts
+ * label_map = {
+ * "testing_count, 1 week ago": ["testing_count", "1 week ago"]
+ * }
+ *
+ * verboseMap = {
+ * testing_count: "Testing Count"
+ * }
+ *
+ * Result:
+ * {
+ * testing_count: "Testing Count",
+ * "testing_count, 1 week ago": "Testing Count, 1 week ago"
+ * }
+ * ```
+ */
+ const newVerboseMap: Record<string, string> = {};
+
+ Object.entries(label_map).forEach(([key, labels]) => {
Review Comment:
**Suggestion:** `addLabelMapToVerboseMap` dereferences `label_map`
unconditionally with `Object.entries(label_map)`. In mixed-timeseries flows,
`label_map` can be absent on query payloads, which will throw at runtime
(`Cannot convert undefined or null to object`) and break chart rendering. Add a
safe default (`{}`) or an early return when `label_map` is falsy. [null pointer]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ MixedTimeseries charts crash when label_map is missing.
- ⚠️ Dashboards with Mixed Chart panels fail to render.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open the MixedTimeseries chart plugin, which wires `transformProps` as
the transformer,
as defined in
`superset-frontend/plugins/plugin-chart-echarts/src/MixedTimeseries/index.ts:21-24,34-86`
where `transformProps` is imported and passed to the `EchartsChartPlugin`
constructor.
2. In
`superset-frontend/plugins/plugin-chart-echarts/src/MixedTimeseries/transformProps.ts:25-34`,
`transformProps` destructures `label_map` from the two query results without
a default:
`const { label_map: labelMap, ... } = queriesData[0] as
TimeseriesChartDataResponseResult;` and `const { label_map: labelMapB, ... }
=
queriesData[1] as TimeseriesChartDataResponseResult;`.
3. When the backend response for either query omits `label_map` (permitted
because
`ChartDataResponseResult` in
`packages/superset-ui-core/src/query/types/QueryResponse.ts:10-18` does not
define
`label_map`, and the MixedTimeseries test helper `createTestQueryData` in
`plugins/plugin-chart-echarts/test/MixedTimeseries/transformProps.test.ts:10-36`
models
`label_map` as optional), the destructured `labelMap` / `labelMapB`
variables become
`undefined`.
4. `transformProps` then passes these potentially undefined values into
`addLabelMapToVerboseMap` at `MixedTimeseries/transformProps.ts:36-40`, and
inside
`addLabelMapToVerboseMap` in
`superset-frontend/plugins/plugin-chart-echarts/src/utils/forecast.ts:90-132`
the line
`Object.entries(label_map).forEach(([key, labels]) => {` executes, causing a
runtime
`TypeError: Cannot convert undefined or null to object` and preventing
MixedTimeseries
charts (and their tooltips) from rendering.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=3f60c89f73714f65bc77bf3eee0daaf1&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=3f60c89f73714f65bc77bf3eee0daaf1&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/utils/forecast.ts
**Line:** 90:132
**Comment:**
*Null Pointer: `addLabelMapToVerboseMap` dereferences `label_map`
unconditionally with `Object.entries(label_map)`. In mixed-timeseries flows,
`label_map` can be absent on query payloads, which will throw at runtime
(`Cannot convert undefined or null to object`) and break chart rendering. Add a
safe default (`{}`) or an early return when `label_map` is falsy.
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%2F37229&comment_hash=4ab7b2c1aa4a61515b8d79bf8f90a4c282fff4522284d42eac9e099013d2de43&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37229&comment_hash=4ab7b2c1aa4a61515b8d79bf8f90a4c282fff4522284d42eac9e099013d2de43&reaction=dislike'>👎</a>
##########
superset-frontend/plugins/plugin-chart-echarts/src/utils/forecast.ts:
##########
@@ -26,6 +26,147 @@ import {
} from '../types';
import { sanitizeHtml } from './series';
+/**
+ * Escapes RegExp metacharacters in a string so it can be safely used in a
+ * dynamically created regular expression.
+ *
+ * @param value - The raw string to escape
+ * @returns The escaped string safe for use in `new RegExp(...)`
+ */
+const escapeRegex = (value: string) =>
+ value.replace(/[.*+?^${}()|[\]\\]/g, '\\$&');
+
+/**
+ * Replaces a label inside a compound key only if it appears as a complete
+ * word/token and contains at least one alphabetic character.
+ *
+ * @param key - The source string (typically a compound field name)
+ * @param label - The label to search for as a standalone token
+ * @param replacement - The human-readable value to replace the label with
+ * @returns The transformed key if a valid match exists, otherwise the
original key
+ */
+const replaceLabelIfExists = (
+ key: string,
+ label: string,
+ replacement: string,
+) => {
+ /**
+ * Logic:
+ *
+ * This function is intentionally stricter than a simple substring replace:
+ * - The label must NOT be part of a larger word (e.g. "account" will NOT
match
+ * "testing_account").
+ * - Underscores (`_`) are treated as part of the word.
+ * - Numeric-only matches are ignored (e.g. "12" will NOT match "123").
+ *
+ * If the label is found, only the matched portion is replaced; otherwise,
+ * the original key is returned unchanged.
+ *
+ * Examples:
+ * - replaceLabelIfExists("testing_account 123", "testing_account",
"Account")
+ * → "Account 123"
+ * - replaceLabelIfExists("testing_account 123", "account", "Account")
+ * → "testing_account 123"
+ * - replaceLabelIfExists("123", "12", "X")
+ * → "123"
+ */
+
+ if (key === label) {
+ return replacement;
+ }
+
+ const escapedLabel = escapeRegex(label);
+ const regex = new RegExp(`(?<!\\w)${escapedLabel}(?!\\w)`, 'g');
+ return regex.test(key) ? key.replace(regex, replacement) : key;
+};
+
+/**
+ * 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:
+ *
+ * This function takes a mapping of compound field names to their component
labels
+ * and replaces those labels with their corresponding human-readable values
from
+ * `verboseMap`, producing display-friendly versions of the compound keys.
+ *
+ * Replacement behavior:
+ * - Each compound key is processed word-by-word (split on spaces).
+ * - Only labels that exist in `verboseMap` are considered.
+ * - Each word is replaced at most once, using `replaceLabelIfExists`, which:
+ * - Matches only full tokens (no partial matches).
+ * - Treats underscores (`_`) as part of a token.
+ * - Is case-sensitive.
+ * - Labels not found in `verboseMap` are left unchanged.
+ *
+ * The original `verboseMap` is preserved and extended with the newly
generated
+ * human-readable entries.
+ *
+ * Example:
+ * ```ts
+ * label_map = {
+ * "testing_count, 1 week ago": ["testing_count", "1 week ago"]
+ * }
+ *
+ * verboseMap = {
+ * testing_count: "Testing Count"
+ * }
+ *
+ * Result:
+ * {
+ * testing_count: "Testing Count",
+ * "testing_count, 1 week ago": "Testing Count, 1 week ago"
+ * }
+ * ```
+ */
+ const newVerboseMap: Record<string, string> = {};
+
+ Object.entries(label_map).forEach(([key, labels]) => {
+ if (labels) {
+ const newLabelMap: Record<string, string> = labels
+ .filter(l => verboseMap[l])
+ .reduce(
+ (acc, label) => ({
+ ...acc,
+ [label]: verboseMap[label],
+ }),
+ {},
+ );
+
+ const newKey = key
+ .split(' ')
+ .map(word => {
+ for (const label of Object.keys(newLabelMap)) {
+ const newWord = replaceLabelIfExists(
+ word,
+ label,
+ newLabelMap[label],
+ );
+
+ if (newWord !== word) {
+ return newWord;
+ }
+ }
+
+ return word;
+ })
+ .join(' ');
Review Comment:
**Suggestion:** The replacement logic tokenizes keys by spaces before trying
to replace labels, so labels containing spaces (for example quoted column names
or metric labels like `"Order Total"`) will never match and remain
untranslated. Perform replacements on the full `key` string (or tokenization
that preserves multi-word labels) so verbose labels with spaces are correctly
applied. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Multi-word metric labels stay untranslated in forecast tooltips.
- ⚠️ Users see raw metric identifiers instead of friendly names.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Consider an input similar to the documented example in
`superset-frontend/plugins/plugin-chart-echarts/src/utils/forecast.ts:83-128`,
but with a
multi-word label: call `addLabelMapToVerboseMap` (forecast.ts:90-168) with
`label_map = {
'"Order Total", 1 week ago': ['"Order Total"', '1 week ago'] }` and
`verboseMap = {
'"Order Total"': 'Order Total (USD)' }`.
2. `addLabelMapToVerboseMap` builds `newLabelMap` from `labels` at
`forecast.ts:134-142`,
so `newLabelMap` contains a key for the multi-word label `"Order Total"`
with value
`'Order Total (USD)'`.
3. The function then computes `newKey` at `forecast.ts:144-161` by splitting
the compound
key on spaces (`key.split(' ')`) and mapping each `word` through
`replaceLabelIfExists`,
so the tokens passed are `"Order`, `Total",`, `1`, `week`, `ago` instead of
the full
`"Order Total"` string.
4. Because `replaceLabelIfExists` (forecast.ts:48-81) compares each `word`
token against
the full label and constructs a regex that also expects the full label, none
of the
space-separated tokens match the multi-word label `"Order Total"`, leaving
`newKey`
unchanged and causing multi-word labels to remain untranslated (or even
overriding an
existing verbose entry) in tooltips and axis labels that rely on the enriched
`verboseMap`.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=0ad98abd37174ee59ea67905bd25bedf&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=0ad98abd37174ee59ea67905bd25bedf&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/utils/forecast.ts
**Line:** 144:161
**Comment:**
*Logic Error: The replacement logic tokenizes keys by spaces before
trying to replace labels, so labels containing spaces (for example quoted
column names or metric labels like `"Order Total"`) will never match and remain
untranslated. Perform replacements on the full `key` string (or tokenization
that preserves multi-word labels) so verbose labels with spaces are correctly
applied.
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%2F37229&comment_hash=2bc2f2eb399ca39ac4c4f63dae0382141106c299e093bd3c1216a5c74433e559&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37229&comment_hash=2bc2f2eb399ca39ac4c4f63dae0382141106c299e093bd3c1216a5c74433e559&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]
