codeant-ai-for-open-source[bot] commented on code in PR #40919:
URL: https://github.com/apache/superset/pull/40919#discussion_r3390469715
##########
superset-frontend/plugins/plugin-chart-echarts/src/Waterfall/transformProps.ts:
##########
@@ -329,7 +329,7 @@ export default function transformProps(
value = row[breakdownName];
}
if (!value) {
- value = NULL_STRING;
+ value = NULL_STRING();
}
Review Comment:
**Suggestion:** This condition treats all falsy x-axis values as null, so
legitimate values like `0` become `<NULL>`. That breaks category labeling and
can produce wrong axis formatting/grouping. Restrict the null substitution to
`null`/`undefined` (and handle empty string separately if needed). [falsy zero
check]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Waterfall x-axis mislabels 0 or empty categories.
- ⚠️ Waterfall chart groups falsy categories into null bucket.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. `MainPreset` registers `EchartsWaterfallChartPlugin` with key
`VizType.Waterfall` in
`superset-frontend/src/visualizations/presets/MainPreset.ts:169-171`, and
`@superset-ui/plugin-chart-echarts` re-exports `WaterfallTransformProps` from
`superset-frontend/plugins/plugin-chart-echarts/src/index.ts:12` pointing to
`./Waterfall/transformProps`.
2. The unit test
`superset-frontend/plugins/plugin-chart-echarts/test/Waterfall/transformProps.test.ts:24-40`
shows how `transformProps` is invoked via `ChartProps` with
`queriesData[0].data` and
formData specifying `x_axis` and `metric`; to reproduce the issue, use a
similar setup but
include a row in `data` where the x-axis column (e.g. `year`) is a
legitimate falsy value
like `0` or `''`.
3. When `transformProps` executes in
`superset-frontend/plugins/plugin-chart-echarts/src/Waterfall/transformProps.ts:159-231`,
it builds `transformedData` via `transformer` (lines 224-231) and then
computes
`xAxisData` at lines 323-339 by mapping each `row` to an x-axis label; for
each row it
sets `value = row[xAxisName]` or `row[breakdownName]` and then reaches the
block at lines
331-333.
4. At line 331, `if (!value) { value = NULL_STRING(); }` treats any falsy
`value` as null,
so a legitimate x-axis value of `0`, `false`, or `''` is converted to
`NULL_STRING()`
instead of remaining `0`/`false`/`''`, and the resulting x-axis categories
(used in
`echartOptions.xAxis.data` at line 446) and tooltip/title formatter
`xAxisFormatter`
(lines 341-351) mislabel these categories as `<NULL>`, merging them into the
null bucket
and breaking category labeling and grouping.
```
</details>
[Fix in
Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=552964cb41294b21a14a4c41411d8f82&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
| [Fix in VSCode
Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=552964cb41294b21a14a4c41411d8f82&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/Waterfall/transformProps.ts
**Line:** 331:333
**Comment:**
*Falsy Zero Check: This condition treats all falsy x-axis values as
null, so legitimate values like `0` become `<NULL>`. That breaks category
labeling and can produce wrong axis formatting/grouping. Restrict the null
substitution to `null`/`undefined` (and handle empty string separately if
needed).
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%2F40919&comment_hash=ce393bdb3e202bda4a2cfd82ab89801b53d677505bf4c95f1167ccc156255428&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40919&comment_hash=ce393bdb3e202bda4a2cfd82ab89801b53d677505bf4c95f1167ccc156255428&reaction=dislike'>👎</a>
##########
superset-frontend/plugins/plugin-chart-echarts/src/Bubble/transformProps.ts:
##########
@@ -145,7 +145,7 @@ export default function transformProps(chartProps:
EchartsBubbleChartProps) {
data.forEach(datum => {
const dataName = bubbleSeries ? datum[bubbleSeries] : datum[entity];
- const name = dataName ? String(dataName) : NULL_STRING;
+ const name = dataName ? String(dataName) : NULL_STRING();
Review Comment:
**Suggestion:** The null fallback uses a truthy check, so valid category
values like `0`, `false`, and `''` are incorrectly converted to the null label.
This will merge distinct categories into the null bucket and produce incorrect
legend/color grouping. Check explicitly for `null`/`undefined` instead of falsy
values. [falsy zero check]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Bubble chart merges 0-category into null legend bucket.
- ⚠️ Bubble legend mislabels falsy category values as null.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. In the frontend, `MainPreset` registers `EchartsBubbleChartPlugin` with
key
`VizType.Bubble` at
`superset-frontend/src/visualizations/presets/MainPreset.ts:198`,
which pulls `BubbleTransformProps` from `@superset-ui/plugin-chart-echarts`
(re-exported
in `superset-frontend/plugins/plugin-chart-echarts/src/index.ts:11` as the
default from
`./Bubble/transformProps`).
2. Prepare bubble chart form data similar to the unit test
(`superset-frontend/plugins/plugin-chart-echarts/test/Bubble/transformProps.test.ts:10-33`),
but use a dataset where the grouping column (e.g. `entity`/`customer_name`)
contains a
legitimate falsy value such as `0` or `''` for one row in
`queriesData[0].data` (see
structure at lines 35-58 in that test).
3. When `transformProps` runs in
`superset-frontend/plugins/plugin-chart-echarts/src/Bubble/transformProps.ts:98-169`,
it
iterates `data.forEach(datum => { ... })` at line 146, computes `dataName =
bubbleSeries ?
datum[bubbleSeries] : datum[entity];` at line 147, and for the row where
`datum[entity]`
is `0` or `''`, `dataName` is that falsy value.
4. At line 148, `const name = dataName ? String(dataName) : NULL_STRING();`
treats
`0`/`''` as falsy, so `name` becomes `NULL_STRING()` instead of `'0'`/`''`,
and the
legend/series name added at lines 151-169 uses the null label; this
collapses the real
`0`/empty-string category into the `<NULL>` bucket, producing incorrect
legend entries and
color grouping for that category.
```
</details>
[Fix in
Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=80a6fbbfc8e2495880999a3db562eff7&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
| [Fix in VSCode
Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=80a6fbbfc8e2495880999a3db562eff7&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/Bubble/transformProps.ts
**Line:** 148:148
**Comment:**
*Falsy Zero Check: The null fallback uses a truthy check, so valid
category values like `0`, `false`, and `''` are incorrectly converted to the
null label. This will merge distinct categories into the null bucket and
produce incorrect legend/color grouping. Check explicitly for
`null`/`undefined` instead of falsy values.
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%2F40919&comment_hash=81d6d294ab2ad46696f8de10b05a3ff0214cd2fda46e76c949a99e50ffe60a60&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40919&comment_hash=81d6d294ab2ad46696f8de10b05a3ff0214cd2fda46e76c949a99e50ffe60a60&reaction=dislike'>👎</a>
##########
superset-frontend/plugins/plugin-chart-echarts/src/Treemap/EchartsTreemap.tsx:
##########
@@ -124,7 +124,7 @@ export default function EchartsTreemap({
const drillToDetailFilters: BinaryQueryObjectFilterClause[] = [];
const drillByFilters: BinaryQueryObjectFilterClause[] = [];
treePath.forEach((path, i) => {
- const val = path === 'null' ? NULL_STRING : path;
+ const val = path === 'null' ? NULL_STRING() : path;
Review Comment:
**Suggestion:** Mapping the literal string `'null'` to the null token
changes semantics for real string values equal to `"null"`, so drill filters
can target the wrong records. Preserve the original value unless you can
reliably detect an actual null sentinel from the source data. [incorrect
condition logic]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Treemap drill-to-detail misfilters literal 'null' category.
- ⚠️ Treemap drill-by filters mis-target literal 'null' values.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. `MainPreset` wires `EchartsTreemapChartPlugin` into the visualization
registry with key
`VizType.Treemap` at
`superset-frontend/src/visualizations/presets/MainPreset.ts:125`, and
the plugin uses `EchartsTreemap` from
`superset-frontend/plugins/plugin-chart-echarts/src/Treemap/EchartsTreemap.tsx`
as its
React chart component.
2. In `Treemap` transform props
(`superset-frontend/plugins/plugin-chart-echarts/src/Treemap/transformProps.ts:170-187`),
leaf node names are derived via `formatSeriesName(...)`; when the underlying
groupby
column holds the literal string `"null"`, `formatSeriesName` (see
`superset-frontend/plugins/plugin-chart-echarts/src/utils/series.ts:41-72`)
returns
`"null"` unchanged because it only special-cases `name === null || name ===
undefined`.
3. During rendering, ECharts supplies `treePathInfo` to event handlers and
`extractTreePathInfo`
(`superset-frontend/plugins/plugin-chart-echarts/src/Treemap/constants.ts:27-36`)
builds
`treePath` from `pathInfo.name`, so the click/contextmenu handlers in
`EchartsTreemap`
receive `treePath` elements equal to `"null"` for those categories.
4. On context menu (right-click) in `EchartsTreemap` at
`superset-frontend/plugins/plugin-chart-echarts/src/Treemap/EchartsTreemap.tsx:117-155`,
the handler iterates `treePath.forEach((path, i) => { ... })` and executes
`const val =
path === 'null' ? NULL_STRING() : path;` at line 127; for a real `"null"`
category this
coerces `val` to `NULL_STRING()` (e.g. `<NULL>`), so the
`drillToDetailFilters` and
`drillByFilters` built at lines 128-137 use `<NULL>` instead of `"null"` as
the filter
value, causing drill-to-detail/drill-by requests to target the null token
rather than the
actual `"null"` string rows.
```
</details>
[Fix in
Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=63a74817f1c44476ab680c1bba2d849c&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
| [Fix in VSCode
Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=63a74817f1c44476ab680c1bba2d849c&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/Treemap/EchartsTreemap.tsx
**Line:** 127:127
**Comment:**
*Incorrect Condition Logic: Mapping the literal string `'null'` to the
null token changes semantics for real string values equal to `"null"`, so drill
filters can target the wrong records. Preserve the original value unless you
can reliably detect an actual null sentinel from the source data.
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%2F40919&comment_hash=5d0882badfb723ec27e548fee9d3ef0e9768dc7eddfcace023be0cf71db16198&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40919&comment_hash=5d0882badfb723ec27e548fee9d3ef0e9768dc7eddfcace023be0cf71db16198&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]
