codeant-ai-for-open-source[bot] commented on code in PR #42247:
URL: https://github.com/apache/superset/pull/42247#discussion_r3616743191
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/EchartsTimeseries.tsx:
##########
@@ -63,10 +69,97 @@ export default function EchartsTimeseries({
onLegendScroll,
}: TimeseriesChartTransformedProps) {
const { stack } = formData;
+ const theme = useTheme();
const echartRef = useRef<EchartsHandler | null>(null);
// eslint-disable-next-line no-param-reassign
refs.echartRef = echartRef;
const clickTimer = useRef<ReturnType<typeof setTimeout>>();
+
+ // Draggable percent-change baseline: when the rebase view is active, a
+ // vertical line is drawn on the plot; dragging it re-indexes every series
+ // to the hovered point via the composable rebase, entirely client-side.
+ const rebaseEnabled = Boolean(
+ (formData as { rebasePercentChange?: boolean }).rebasePercentChange,
+ );
+ useEffect(() => {
+ if (!rebaseEnabled) return undefined;
+ const chart = echartRef.current?.getEchartInstance?.();
+ if (!chart) return undefined;
+
+ const option = chart.getOption() as {
+ series?: { data?: SeriesDataPoint[] }[];
+ };
+ const baseSeries = (option.series ?? []).map(s =>
+ Array.isArray(s.data) ? (s.data as SeriesDataPoint[]) : [],
+ );
+ const xs = Array.from(
+ new Set(baseSeries.flat().map(([x]) => Number(x))),
+ ).sort((a, b) => a - b);
Review Comment:
**Suggestion:** The x-values are coerced with `Number(x)` before snapping,
which turns common datetime strings (for example ISO timestamps) into `NaN`;
that makes baseline initialization/snap logic invalid and can break dragging
behavior. Normalize/parsing should preserve valid time values instead of blind
numeric coercion. [type error]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ Percent-change baseline fails on string-based time axes.
- ⚠️ Dragging baseline may not update series correctly.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open a Time-series Line chart in Explore that uses the ECharts renderer,
which mounts
`EchartsTimeseries` from
`superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/EchartsTimeseries.tsx:50`
for the visualization.
2. Configure the chart so the x-axis values in the underlying `series.data`
are datetime
strings (for example ISO timestamps returned under `DTTM_ALIAS`), not
pre-parsed numbers;
these are read back from the chart via `chart.getOption()` in the effect at
lines 89–93.
3. Enable the Percent change / rebase view so `rebaseEnabled` at lines 81–83
is true,
causing the `useEffect` at lines 84–162 to run and compute `xs` at lines
95–97 via
`baseSeries.flat().map(([x]) => Number(x))`.
4. Observe that `Number(x)` on string timestamps yields `NaN`, so `xs`
contains `NaN`
values and `baselineX = xs[0]` at line 99 becomes `NaN`; the subsequent
`chart.convertToPixel({ xAxisIndex: 0 }, baselineX)` in `drawHandle()` at
lines 110–115
fails and the draggable baseline either does not render or behaves
incorrectly when
dragging.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=2658bc4c99a844ca8806e66195e88040&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=2658bc4c99a844ca8806e66195e88040&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/Timeseries/EchartsTimeseries.tsx
**Line:** 95:97
**Comment:**
*Type Error: The x-values are coerced with `Number(x)` before snapping,
which turns common datetime strings (for example ISO timestamps) into `NaN`;
that makes baseline initialization/snap logic invalid and can break dragging
behavior. Normalize/parsing should preserve valid time values instead of blind
numeric coercion.
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%2F42247&comment_hash=2c6aa33c2cbe97774f72bcdcf8eb45cbefbd95c8a3ad781ad773ac6481c3ae4d&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42247&comment_hash=2c6aa33c2cbe97774f72bcdcf8eb45cbefbd95c8a3ad781ad773ac6481c3ae4d&reaction=dislike'>👎</a>
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/EchartsTimeseries.tsx:
##########
@@ -63,10 +69,97 @@ export default function EchartsTimeseries({
onLegendScroll,
}: TimeseriesChartTransformedProps) {
const { stack } = formData;
+ const theme = useTheme();
const echartRef = useRef<EchartsHandler | null>(null);
// eslint-disable-next-line no-param-reassign
refs.echartRef = echartRef;
const clickTimer = useRef<ReturnType<typeof setTimeout>>();
+
+ // Draggable percent-change baseline: when the rebase view is active, a
+ // vertical line is drawn on the plot; dragging it re-indexes every series
+ // to the hovered point via the composable rebase, entirely client-side.
+ const rebaseEnabled = Boolean(
+ (formData as { rebasePercentChange?: boolean }).rebasePercentChange,
+ );
+ useEffect(() => {
+ if (!rebaseEnabled) return undefined;
+ const chart = echartRef.current?.getEchartInstance?.();
+ if (!chart) return undefined;
+
+ const option = chart.getOption() as {
+ series?: { data?: SeriesDataPoint[] }[];
+ };
+ const baseSeries = (option.series ?? []).map(s =>
+ Array.isArray(s.data) ? (s.data as SeriesDataPoint[]) : [],
+ );
+ const xs = Array.from(
+ new Set(baseSeries.flat().map(([x]) => Number(x))),
+ ).sort((a, b) => a - b);
+ if (xs.length === 0) return undefined;
+ let baselineX = xs[0];
Review Comment:
**Suggestion:** The baseline position is stored in a local variable inside
the effect, so any dependency-triggered rerun resets it to the first x-value
and discards the user's dragged baseline. Persist the selected baseline in
React state/ref and reapply it after rerenders/resizes. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ User-chosen baseline resets on resize or option changes.
- ⚠️ Rebasing state cannot persist across chart re-renders.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Render a Percent change Line chart so that `EchartsTimeseries` (file
`superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/EchartsTimeseries.tsx:50`)
receives `formData.rebasePercentChange` true, making `rebaseEnabled` true at
lines 81–83
and activating the baseline effect.
2. Drag the vertical baseline handle to a later x-position; the `ondrag`
handler at lines
130–139 snaps to a new x via `snapToNearestX(xs, dataX)` and calls
`applyBaseline(snapped)` at line 138, which updates the series and assigns
the new value
to the local `baselineX` variable at line 102.
3. Cause a prop change that affects the effect dependencies, such as
resizing the chart
container (changing `width`/`height` passed into `EchartsTimeseries` at
lines 52–54) or
changing `echartOptions` by interacting with controls; the `useEffect` at
lines 84–162
depends on `[rebaseEnabled, echartOptions, width, height, theme]` (line 162)
and therefore
reruns.
4. On rerun, `xs` is recomputed at lines 95–97 and `baselineX` is reset to
`xs[0]` at line
99 before `drawHandle()` is called, so the handle is redrawn at the first
x-value and the
previously user-selected baseline position is lost.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=b16b823ef5d441b0a20cd3d8dfbf4b0e&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=b16b823ef5d441b0a20cd3d8dfbf4b0e&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/Timeseries/EchartsTimeseries.tsx
**Line:** 99:99
**Comment:**
*Logic Error: The baseline position is stored in a local variable
inside the effect, so any dependency-triggered rerun resets it to the first
x-value and discards the user's dragged baseline. Persist the selected baseline
in React state/ref and reapply it after rerenders/resizes.
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%2F42247&comment_hash=28f6901a2ec7a0b646f23a3131a62f2691d5783233d4a4c0ace5440fa810311a&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42247&comment_hash=28f6901a2ec7a0b646f23a3131a62f2691d5783233d4a4c0ace5440fa810311a&reaction=dislike'>👎</a>
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/EchartsTimeseries.tsx:
##########
@@ -63,10 +69,97 @@ export default function EchartsTimeseries({
onLegendScroll,
}: TimeseriesChartTransformedProps) {
const { stack } = formData;
+ const theme = useTheme();
const echartRef = useRef<EchartsHandler | null>(null);
// eslint-disable-next-line no-param-reassign
refs.echartRef = echartRef;
const clickTimer = useRef<ReturnType<typeof setTimeout>>();
+
+ // Draggable percent-change baseline: when the rebase view is active, a
+ // vertical line is drawn on the plot; dragging it re-indexes every series
+ // to the hovered point via the composable rebase, entirely client-side.
+ const rebaseEnabled = Boolean(
+ (formData as { rebasePercentChange?: boolean }).rebasePercentChange,
+ );
+ useEffect(() => {
+ if (!rebaseEnabled) return undefined;
+ const chart = echartRef.current?.getEchartInstance?.();
+ if (!chart) return undefined;
+
+ const option = chart.getOption() as {
+ series?: { data?: SeriesDataPoint[] }[];
+ };
+ const baseSeries = (option.series ?? []).map(s =>
+ Array.isArray(s.data) ? (s.data as SeriesDataPoint[]) : [],
+ );
+ const xs = Array.from(
+ new Set(baseSeries.flat().map(([x]) => Number(x))),
+ ).sort((a, b) => a - b);
+ if (xs.length === 0) return undefined;
+ let baselineX = xs[0];
+
+ const applyBaseline = (newX: number) => {
+ baselineX = newX;
+ chart.setOption({
+ series: baseSeries.map(data => ({
+ data: rebaseSeriesData(data, newX),
+ })),
+ });
+ };
+
+ const drawHandle = () => {
+ const gridRect = { top: 0, height: chart.getHeight() };
+ let px: number;
+ try {
+ [px] = [chart.convertToPixel({ xAxisIndex: 0 }, baselineX) as number];
+ } catch {
+ return;
+ }
+ chart.setOption({
+ graphic: [
+ {
+ id: 'percent-change-baseline',
+ type: 'rect',
+ x: px - 4,
+ y: gridRect.top,
+ shape: { width: 8, height: gridRect.height },
+ style: { fill: theme.colorFillQuaternary },
+ cursor: 'ew-resize',
+ draggable: true,
+ z: 100,
+ ondrag(this: { x: number; y: number }) {
+ this.y = gridRect.top;
+ const dataX = chart.convertFromPixel(
+ { xAxisIndex: 0 },
+ this.x + 4,
+ ) as number;
+ const snapped = snapToNearestX(xs, dataX);
+ if (snapped !== undefined && snapped !== baselineX) {
+ applyBaseline(snapped);
+ }
Review Comment:
**Suggestion:** Rebasing every series on every `ondrag` event causes
repeated full `setOption` updates while the pointer moves, which can freeze or
stutter large charts. Throttle/debounce drag updates (or apply on drag end with
lightweight preview) to avoid a high-frequency full-series recomputation loop.
[possible bug]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Dragging baseline on large charts visibly stutters.
- ⚠️ Interactive rebasing feels laggy for heavy datasets.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Render a Percent change time-series chart using the ECharts Line chart so
that
`EchartsTimeseries` in
`superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/EchartsTimeseries.tsx:50`
is mounted with `rebaseEnabled` true (lines 81–83).
2. Use a dataset that produces many series points (large `baseSeries` array
built at lines
92–94) so rebasing involves substantial data (thousands of points across
series).
3. Drag the vertical baseline handle drawn by `drawHandle()` at lines
110–155; during the
drag, ECharts continuously invokes the `ondrag` handler defined at lines
130–139.
4. For each drag frame where `snapToNearestX(xs, dataX)` at line 136 returns
a new value,
`applyBaseline(snapped)` at line 138 runs, calling `chart.setOption({ series:
baseSeries.map(...) })` at lines 101–107; this repeated full-series
recomputation and
re-render on a hot interaction path causes visible stutter or freezing while
the baseline
is dragged.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=a16723fae8ca46fa967e3d0f95c66a16&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=a16723fae8ca46fa967e3d0f95c66a16&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/Timeseries/EchartsTimeseries.tsx
**Line:** 130:139
**Comment:**
*Possible Bug: Rebasing every series on every `ondrag` event causes
repeated full `setOption` updates while the pointer moves, which can freeze or
stutter large charts. Throttle/debounce drag updates (or apply on drag end with
lightweight preview) to avoid a high-frequency full-series recomputation loop.
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%2F42247&comment_hash=9c3949af6aca6a97e7313f4b0c7bc708dc68685bdef70603b00727ad87090815&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42247&comment_hash=9c3949af6aca6a97e7313f4b0c7bc708dc68685bdef70603b00727ad87090815&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]
