codeant-ai-for-open-source[bot] commented on code in PR #37768:
URL: https://github.com/apache/superset/pull/37768#discussion_r2780769574
##########
superset-frontend/plugins/plugin-chart-echarts/src/Funnel/types.ts:
##########
@@ -64,8 +64,8 @@ export const DEFAULT_FORM_DATA: EchartsFunnelFormData = {
...DEFAULT_LEGEND_FORM_DATA,
groupby: [],
labelLine: false,
- labelType: EchartsFunnelLabelTypeType.Key,
- defaultTooltipLabel: EchartsFunnelLabelTypeType.KeyValuePercent,
+ labelType: EchartsFunnelLabelType.Key,
+ defaultTooltipLabel: EchartsFunnelLabelType.KeyValuePercent,
Review Comment:
**Suggestion:** The default form data does not set a value for the
`tooltipLabelType` field, even though `EchartsFunnelFormData` expects it and
`transformProps` uses it to index the `EchartsFunnelLabelType` enum; when older
or minimal form data is merged with `DEFAULT_FORM_DATA`, `tooltipLabelType` can
be `undefined`, causing a runtime error when
`EchartsFunnelLabelType[tooltipLabelType]` is used and `.includes` is called on
the resulting value. To prevent this, ensure `DEFAULT_FORM_DATA` initializes
`tooltipLabelType` with the same default as `defaultTooltipLabel`. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Funnel chart tooltips crash without tooltip label type.
- ⚠️ Older Funnel charts without tooltip field fail rendering.
```
</details>
```suggestion
tooltipLabelType: EchartsFunnelLabelType.KeyValuePercent,
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Trigger rendering of a Funnel chart (using `EchartsFunnelChartPlugin` in
`superset-frontend/plugins/plugin-chart-echarts/src/Funnel/index.ts:31-34`)
with form data
that omits the `tooltip_label_type` control (e.g. by posting chart data
using the schema
in `superset/examples/featured_charts/charts/Funnel.yaml` but removing line
47
`tooltip_label_type: 5`).
2. The chart plugin calls `transformProps` in
`superset-frontend/plugins/plugin-chart-echarts/src/Funnel/transformProps.ts:87-100`,
passing `chartProps.formData` that does not contain `tooltipLabelType` and
relying on
defaults.
3. Inside `transformProps`, the form data is destructured at
`transformProps.ts:104-129`
as `const { ..., tooltipLabelType, ... }: EchartsFunnelFormData = {
...DEFAULT_LEGEND_FORM_DATA, ...DEFAULT_FUNNEL_FORM_DATA, ...formData };`
while
`DEFAULT_FUNNEL_FORM_DATA` (alias of `DEFAULT_FORM_DATA` defined in
`superset-frontend/plugins/plugin-chart-echarts/src/Funnel/types.ts:63-76`)
does not
define `tooltipLabelType`, so `tooltipLabelType` becomes `undefined`.
4. Later in `transformProps.ts:280-297`, the tooltip formatter executes
`const enumName =
EchartsFunnelLabelType[tooltipLabelType];` and then
`enumName.includes('Key')` /
`enumName.includes('Value')`; with `tooltipLabelType === undefined`,
`enumName` is
`undefined` and calling `.includes` throws a runtime TypeError, causing the
Funnel chart
tooltip (and typically the whole chart render) to fail.
```
</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/Funnel/types.ts
**Line:** 68:68
**Comment:**
*Logic Error: The default form data does not set a value for the
`tooltipLabelType` field, even though `EchartsFunnelFormData` expects it and
`transformProps` uses it to index the `EchartsFunnelLabelType` enum; when older
or minimal form data is merged with `DEFAULT_FORM_DATA`, `tooltipLabelType` can
be `undefined`, causing a runtime error when
`EchartsFunnelLabelType[tooltipLabelType]` is used and `.includes` is called on
the resulting value. To prevent this, ensure `DEFAULT_FORM_DATA` initializes
`tooltipLabelType` with the same default as `defaultTooltipLabel`.
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>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37768&comment_hash=15406c3ac9c846ba4547f2dc20580144240ad38f59e598a754a3cc0f43b47e89&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37768&comment_hash=15406c3ac9c846ba4547f2dc20580144240ad38f59e598a754a3cc0f43b47e89&reaction=dislike'>👎</a>
##########
superset-frontend/plugins/plugin-chart-echarts/src/Funnel/transformProps.ts:
##########
@@ -283,7 +283,7 @@ export default function transformProps(
percentCalculationType,
});
const row = [];
- const enumName = EchartsFunnelLabelTypeType[tooltipLabelType];
+ const enumName = EchartsFunnelLabelType[tooltipLabelType];
Review Comment:
**Suggestion:** If `tooltipLabelType` is ever undefined (for example on
older saved charts that predate this control), then
`EchartsFunnelLabelType[tooltipLabelType]` evaluates to `undefined` and calling
`.includes` on it will throw at runtime, breaking tooltip rendering; adding a
safe fallback to the enum default avoids this crash. [possible bug]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Funnel chart tooltip hover can throw runtime TypeError.
- ⚠️ Explore view for affected funnel charts shows broken tooltips.
- ⚠️ Dashboards embedding such charts log frontend JS errors.
```
</details>
```suggestion
const enumName = EchartsFunnelLabelType[
tooltipLabelType ?? DEFAULT_FUNNEL_FORM_DATA.defaultTooltipLabel
];
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Note the form data type for the funnel chart in
`superset-frontend/plugins/plugin-chart-echarts/src/Funnel/types.ts:31-46`,
where
`EchartsFunnelFormData` declares a required `tooltipLabelType:
EchartsFunnelLabelType`
field (line 37) but this is only a TypeScript constraint, not a runtime
guarantee.
2. Observe the default form data in
`superset-frontend/plugins/plugin-chart-echarts/src/Funnel/types.ts:62-76`:
`DEFAULT_FORM_DATA` defines `defaultTooltipLabel:
EchartsFunnelLabelType.KeyValuePercent`
(line 68) but does not define a default for `tooltipLabelType`.
3. In
`superset-frontend/plugins/plugin-chart-echarts/src/Funnel/transformProps.ts:104-130`,
see
how runtime `formData` is built: `const { ... tooltipLabelType, ... }:
EchartsFunnelFormData = { ...DEFAULT_LEGEND_FORM_DATA,
...DEFAULT_FUNNEL_FORM_DATA,
...formData }`. Because `DEFAULT_FUNNEL_FORM_DATA` (aliased
`DEFAULT_FORM_DATA`) has no
`tooltipLabelType` key, any chart whose backend `formData` omits
`tooltip_label_type`
(e.g., a saved funnel slice created or programmatically stored without that
field) will
produce `tooltipLabelType === undefined` at runtime.
4. When rendering such a funnel chart (e.g., opening it in Explore or on a
dashboard so
that `transformProps` runs), hover any funnel segment to trigger the tooltip
formatter at
`superset-frontend/plugins/plugin-chart-echarts/src/Funnel/transformProps.ts:271-298`.
The
formatter computes `const enumName =
EchartsFunnelLabelType[tooltipLabelType];` at line
286, which evaluates to `undefined` when `tooltipLabelType` is missing. The
next line
calls `enumName.includes('Key')`, causing `TypeError: Cannot read properties
of undefined
(reading 'includes')` and breaking tooltip rendering for that chart.
```
</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/Funnel/transformProps.ts
**Line:** 286:286
**Comment:**
*Possible Bug: If `tooltipLabelType` is ever undefined (for example on
older saved charts that predate this control), then
`EchartsFunnelLabelType[tooltipLabelType]` evaluates to `undefined` and calling
`.includes` on it will throw at runtime, breaking tooltip rendering; adding a
safe fallback to the enum default avoids this crash.
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>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37768&comment_hash=2b1b1e2af53bcc9332f5b390653b0137dd5fd33a85195fdd6a3dc740aacd974a&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37768&comment_hash=2b1b1e2af53bcc9332f5b390653b0137dd5fd33a85195fdd6a3dc740aacd974a&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]
