codeant-ai-for-open-source[bot] commented on code in PR #36689:
URL: https://github.com/apache/superset/pull/36689#discussion_r2625746905
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/EchartsTimeseries.tsx:
##########
@@ -50,52 +49,54 @@ export default function EchartsTimeseries({
setControlValue,
legendData = [],
onContextMenu,
- onLegendStateChanged,
- onFocusedSeries,
xValueFormatter,
xAxis,
refs,
emitCrossFilters,
coltypeMapping,
- onLegendScroll,
+ isHorizontal,
Review Comment:
**Suggestion:** The newly added `isHorizontal` prop is never used; leaving
an unused prop in the function signature may trigger linter warnings and is
misleading—rename it to `_isHorizontal` (or remove it) to signal it's
intentionally unused. [possible bug]
**Severity Level:** Critical 🚨
```suggestion
_isHorizontal,
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
The prop `isHorizontal` is indeed declared in the destructured props but
never referenced anywhere in the component. Renaming to `_isHorizontal` (or
removing it) is a small, correct change that avoids linter warnings and
clarifies intent. This is a harmless, localized cleanup.
</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/Timeseries/EchartsTimeseries.tsx
**Line:** 57:57
**Comment:**
*Possible Bug: The newly added `isHorizontal` prop is never used;
leaving an unused prop in the function signature may trigger linter warnings
and is misleading—rename it to `_isHorizontal` (or remove it) to signal it's
intentionally unused.
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/Timeseries/EchartsTimeseries.tsx:
##########
@@ -164,11 +165,69 @@ export default function EchartsTimeseries({
[emitCrossFilters, setDataMask, getCrossFilterDataMask],
);
- const eventHandlers: EventHandlers = {
- click: props => {
- if (!hasDimensions) {
+ const handleBrushSelected = useCallback(
+ (params: any) => {
+ // Only handle brush events for time axis charts
+ if (xAxis.type !== AxisType.time) {
+ return;
+ }
+
+ // Disable brush selection on touch devices to avoid interfering with
scrolling
+ const isTouchDevice =
+ 'ontouchstart' in window || navigator.maxTouchPoints > 0;
Review Comment:
**Suggestion:** Accessing `window` and `navigator` directly will throw
during server-side rendering (when `window`/`navigator` is undefined). Add
guards using `typeof window !== 'undefined'` and `typeof navigator !==
'undefined'` before checking `ontouchstart` or `maxTouchPoints`. [possible bug]
**Severity Level:** Critical 🚨
```suggestion
(typeof window !== 'undefined' && 'ontouchstart' in window) ||
(typeof navigator !== 'undefined' && navigator.maxTouchPoints > 0);
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
The handler reads global browser APIs; while this code runs only when the
brush event fires in the browser, guarding with `typeof window !== 'undefined'`
and `typeof navigator !== 'undefined'` is a robust defensive change that
prevents potential ReferenceErrors in non-browser environments (SSR/test
runners). The suggested improved code is sensible and low-risk.
</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/Timeseries/EchartsTimeseries.tsx
**Line:** 177:177
**Comment:**
*Possible Bug: Accessing `window` and `navigator` directly will throw
during server-side rendering (when `window`/`navigator` is undefined). Add
guards using `typeof window !== 'undefined'` and `typeof navigator !==
'undefined'` before checking `ontouchstart` or `maxTouchPoints`.
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/Timeseries/transformProps.ts:
##########
@@ -729,32 +531,31 @@ export default function transformProps(
start: TIMESERIES_CONSTANTS.dataZoomStart,
end: TIMESERIES_CONSTANTS.dataZoomEnd,
bottom: TIMESERIES_CONSTANTS.zoomBottom,
- yAxisIndex: isHorizontal ? 0 : undefined,
- },
- {
- type: 'inside',
- yAxisIndex: 0,
- zoomOnMouseWheel: false,
- moveOnMouseWheel: true,
- },
- {
- type: 'inside',
- xAxisIndex: 0,
- zoomOnMouseWheel: false,
- moveOnMouseWheel: true,
},
]
: [],
+ brush:
+ xAxisType === AxisType.time
+ ? {
+ toolbox: [],
Review Comment:
**Suggestion:** Configuration risk: `brush.toolbox` was set to an empty
array (`[]`), but the ECharts brush `toolbox` option expects an object (or to
be omitted) — using an invalid type can lead to runtime errors in some ECharts
versions; remove or set it to an object/undefined to avoid invalid
configuration. [possible bug]
**Severity Level:** Critical 🚨
```suggestion
toolbox: undefined,
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
The brush.toolbox in ECharts expects an object (or omitted/undefined).
Passing an array is likely an invalid config and can cause runtime warnings or
errors. Replacing it with undefined (or a proper object) is a safe, small fix
that prevents malformed options from being passed to ECharts.
</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/Timeseries/transformProps.ts
**Line:** 540:540
**Comment:**
*Possible Bug: Configuration risk: `brush.toolbox` was set to an empty
array (`[]`), but the ECharts brush `toolbox` option expects an object (or to
be omitted) — using an invalid type can lead to runtime errors in some ECharts
versions; remove or set it to an object/undefined to avoid invalid
configuration.
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/test/Timeseries/EchartsTimeseries.test.tsx:
##########
@@ -0,0 +1,354 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { AxisType } from '@superset-ui/core';
+import { EchartsTimeseriesFormData } from '../../src/Timeseries/types';
+
+describe('EchartsTimeseries handleBrushSelected', () => {
+ const mockSetDataMask = jest.fn();
+ const mockXValueFormatter = jest.fn((val: number) => new
Date(val).toISOString());
+
+ const baseFormData: Partial<EchartsTimeseriesFormData> = {
+ granularitySqla: 'ds',
+ };
+
+ const baseXAxis = {
+ label: '__timestamp',
Review Comment:
**Suggestion:** The test uses the string literal '__timestamp' in multiple
places which duplicates the real code's DTTM alias; define and use a single
local constant `DTTM_ALIAS` so the test stays correct if the alias is ever
refactored and to avoid divergence between test and implementation. [logic
error]
**Severity Level:** Minor ⚠️
```suggestion
const DTTM_ALIAS = '__timestamp';
const baseXAxis = {
label: DTTM_ALIAS,
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
Good, harmless suggestion. Centralizing the DTTM alias in a test reduces
duplication and the risk of the test diverging from the implementation
constant. It doesn't fix a bug but improves maintainability and makes future
refactors simpler.
</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/test/Timeseries/EchartsTimeseries.test.tsx
**Line:** 30:31
**Comment:**
*Logic Error: The test uses the string literal '__timestamp' in
multiple places which duplicates the real code's DTTM alias; define and use a
single local constant `DTTM_ALIAS` so the test stays correct if the alias is
ever refactored and to avoid divergence between test and implementation.
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/test/Timeseries/EchartsTimeseries.test.tsx:
##########
@@ -0,0 +1,354 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { AxisType } from '@superset-ui/core';
+import { EchartsTimeseriesFormData } from '../../src/Timeseries/types';
+
+describe('EchartsTimeseries handleBrushSelected', () => {
+ const mockSetDataMask = jest.fn();
+ const mockXValueFormatter = jest.fn((val: number) => new
Date(val).toISOString());
+
+ const baseFormData: Partial<EchartsTimeseriesFormData> = {
+ granularitySqla: 'ds',
+ };
+
+ const baseXAxis = {
+ label: '__timestamp',
+ type: AxisType.time,
+ };
+
+ beforeEach(() => {
+ mockSetDataMask.mockClear();
+ mockXValueFormatter.mockClear();
+ });
+
+ describe('handleBrushSelected logic', () => {
+ // Simulating the handler logic since we can't easily test React hooks
directly
+ const handleBrushSelected = (
+ params: any,
+ xAxis: { label: string; type: AxisType },
+ formData: Partial<EchartsTimeseriesFormData>,
+ setDataMask: jest.Mock,
+ xValueFormatter: (val: number) => string,
+ isTouchDevice = false,
+ ) => {
+ // Only handle brush events for time axis charts
+ if (xAxis.type !== AxisType.time) {
+ return;
+ }
+
+ // Disable brush selection on touch devices
+ if (isTouchDevice) {
+ return;
+ }
+
+ // Get the brush areas from the event
+ const brushAreas = params.batch?.[0]?.areas || [];
+ if (brushAreas.length === 0) {
+ // Brush was cleared, reset the filter
+ setDataMask({
+ extraFormData: {},
+ filterState: {
+ value: null,
+ selectedValues: null,
+ },
+ });
+ return;
+ }
+
+ const area = brushAreas[0];
+ const coordRange = area.coordRange;
+ if (!coordRange || !coordRange[0] || coordRange[0].length < 2) {
+ return;
+ }
+
+ const [startValue, endValue] = coordRange[0];
+
+ const col =
+ xAxis.label === '__timestamp' ? formData.granularitySqla : xAxis.label;
+ const startFormatted = xValueFormatter(startValue);
+ const endFormatted = xValueFormatter(endValue);
Review Comment:
**Suggestion:** `xValueFormatter` is typed to accept a number but
`coordRange` values may arrive as strings from the chart; coerce `startValue`
and `endValue` to numbers before passing to the formatter to avoid type errors
or unexpected formatting results. [type error]
**Severity Level:** Minor ⚠️
```suggestion
const startFormatted = xValueFormatter(Number(startValue));
const endFormatted = xValueFormatter(Number(endValue));
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
Coercing to Number before calling a formatter typed for numbers is
reasonable defensive coding — it avoids a runtime type mismatch if coordRange
can sometimes be strings. It's harmless in tests and can catch cases where the
chart library emits strings. Note this diverges slightly from the production
handler which doesn't coerce; if you want exact parity you could also update
production code, but as a test hardening it's acceptable.
</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/test/Timeseries/EchartsTimeseries.test.tsx
**Line:** 84:85
**Comment:**
*Type Error: `xValueFormatter` is typed to accept a number but
`coordRange` values may arrive as strings from the chart; coerce `startValue`
and `endValue` to numbers before passing to the formatter to avoid type errors
or unexpected formatting results.
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]
