codeant-ai-for-open-source[bot] commented on code in PR #37107:
URL: https://github.com/apache/superset/pull/37107#discussion_r2739549301
##########
superset-frontend/src/components/Chart/ChartRenderer.tsx:
##########
@@ -123,8 +236,8 @@ class ChartRenderer extends Component {
onFilterMenuOpen: this.props.onFilterMenuOpen,
onFilterMenuClose: this.props.onFilterMenuClose,
onLegendStateChanged: this.handleLegendStateChanged,
- setDataMask: dataMask => {
- this.props.actions?.updateDataMask(this.props.chartId, dataMask);
+ setDataMask: (dataMask: DataMask) => {
+ this.props.actions?.updateDataMask?.(this.props.chartId, dataMask);
Review Comment:
**Suggestion:** The `setDataMask` hook currently ignores the `setDataMask`
prop and only calls the optional `actions.updateDataMask`, so in contexts where
`setDataMask` is provided but `actions.updateDataMask` is not, data mask
updates from interactive charts will be silently dropped and
cross-filter/native filter behavior will not work as expected; the hook should
prefer the callback prop when present, and fall back to the action only when
the prop is absent. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Plugins' cross-filter events silently drop data masks.
- ❌ Native filters won't update after interactive chart actions.
- ⚠️ Affects SuperChart plugins (plugins/plugin-chart-echarts).
```
</details>
```suggestion
if (this.props.setDataMask) {
this.props.setDataMask(dataMask);
} else {
this.props.actions?.updateDataMask?.(this.props.chartId, dataMask);
}
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Render a SuperChart that uses ChartRenderer as its wrapper. ChartRenderer
constructs
hooks in its constructor at
superset-frontend/src/components/Chart/ChartRenderer.tsx:229-244 and sets
hooks.setDataMask to a function that calls actions.updateDataMask only (see
the existing
code at lines 239-241).
2. Use a chart plugin that calls hooks.setDataMask when emitting
cross-filter events. For
example, the ECharts timeseries plugin calls setDataMask in
plugins/plugin-chart-echarts/src/Timeseries/EchartsTimeseries.tsx:162 (and
transformProps
uses const { setDataMask = () => {} } = hooks at
plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts:569 and
plugins/plugin-chart-echarts/src/Gauge/transformProps.ts:229).
3. Provide a setDataMask callback via the ChartRenderer prop
(ChartRendererProps.setDataMask) from the parent component (some consumers
or tests pass
hooks.setDataMask directly; tests and transformProps rely on
hooks.setDataMask - see many
plugin usages returned by Grep). However, do NOT provide
actions.updateDataMask in
props.actions (or provide an actions object without updateDataMask).
4. Trigger an interactive cross-filter or plugin event that calls
hooks.setDataMask.
Because ChartRenderer's hook implementation unconditionally calls
this.props.actions?.updateDataMask?.(...), the supplied
this.props.setDataMask callback
will not be invoked and the data mask update will be silently dropped.
Observable result:
no cross-filter/native filter update occurs even though a setDataMask
callback was
provided (relevant plugin files: plugins/plugin-chart-echarts/*
transformProps and
Echarts* components that call setDataMask; ChartRenderer hook code at
src/components/Chart/ChartRenderer.tsx:239-241).
5. Explanation why this reproduces on real code: Grep across the codebase
shows many
plugins expect hooks.setDataMask to be callable (examples at
plugins/plugin-chart-echarts/*,
plugins/plugin-chart-table/src/TableChart.tsx, and chart
customization components). ChartRenderer currently prefers
actions.updateDataMask and
ignores the prop callback, which is inconsistent with callers that expect
hooks.setDataMask to forward to a provided callback.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/components/Chart/ChartRenderer.tsx
**Line:** 240:240
**Comment:**
*Logic Error: The `setDataMask` hook currently ignores the
`setDataMask` prop and only calls the optional `actions.updateDataMask`, so in
contexts where `setDataMask` is provided but `actions.updateDataMask` is not,
data mask updates from interactive charts will be silently dropped and
cross-filter/native filter behavior will not work as expected; the hook should
prefer the callback prop when present, and fall back to the action only when
the prop is absent.
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]
