Copilot commented on code in PR #36128:
URL: https://github.com/apache/superset/pull/36128#discussion_r2539247606
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts:
##########
@@ -372,6 +391,38 @@ export default function transformProps(
}
}
});
+
+ // ----- ensure series data are sorted naturally on the x-value -----
+// Run after all series have been created so each series.data is complete.
Review Comment:
The comment formatting is inconsistent. The comment on line 395 should start
with `//` on the same line as the code block begins, or the entire comment
block (lines 395-396) should be properly indented. Currently, line 395 has
trailing whitespace and line 396 starts a new comment without proper
indentation alignment.
```suggestion
// Run after all series have been created so each series.data is complete.
```
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts:
##########
@@ -372,6 +391,38 @@ export default function transformProps(
}
}
});
+
+ // ----- ensure series data are sorted naturally on the x-value -----
+// Run after all series have been created so each series.data is complete.
+series.forEach((s: SeriesOption) => {
+ const dataArr = (s as any).data;
+ if (!Array.isArray(dataArr) || dataArr.length <= 1) return;
+
+ (s as any).data = dataArr.sort((row1: any, row2: any) => {
+ // extract the raw x values (support both [x,y] and { x, y } shapes)
+ const rawX1 = Array.isArray(row1) ? row1[0] : row1?.x;
+ const rawX2 = Array.isArray(row2) ? row2[0] : row2?.x;
+
+ // If this chart's x-axis is temporal, coerce to timestamps (numbers) for
sorting.
+ // Fallback to original raw values if parsing fails.
+ const getComparableX = (raw: any) => {
+ if (xAxisType === AxisType.Time) {
+ // If it's already a number, use it. Otherwise try to coerce to Date
timestamp.
+ if (typeof raw === 'number' && isFinite(raw)) return raw;
+ const parsed = new Date(String(raw)).getTime();
+ return isFinite(parsed) ? parsed : String(raw);
+ }
+ return raw;
Review Comment:
The `getComparableX` function has inconsistent return types. When `xAxisType
=== AxisType.Time`, it can return either a number (timestamp) or a string
(fallback). For non-temporal axes, it returns the raw value which could be any
type. This type inconsistency makes the sorting behavior less predictable.
Consider explicitly documenting the expected return types or normalizing all
values to a consistent comparable type.
```suggestion
// For non-temporal axes, always coerce to string for consistent
comparison
return String(raw);
```
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts:
##########
@@ -344,7 +363,7 @@ export default function transformProps(
metrics,
labelMap?.[seriesName]?.[0],
) ?? defaultFormatter),
- showValue,
+ showValue:Boolean(showValue),
Review Comment:
Missing space after colon. Should be `showValue: Boolean(showValue)` with a
space for consistent formatting.
```suggestion
showValue: Boolean(showValue),
```
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts:
##########
@@ -372,6 +391,38 @@ export default function transformProps(
}
}
});
+
+ // ----- ensure series data are sorted naturally on the x-value -----
+// Run after all series have been created so each series.data is complete.
+series.forEach((s: SeriesOption) => {
+ const dataArr = (s as any).data;
+ if (!Array.isArray(dataArr) || dataArr.length <= 1) return;
+
+ (s as any).data = dataArr.sort((row1: any, row2: any) => {
+ // extract the raw x values (support both [x,y] and { x, y } shapes)
+ const rawX1 = Array.isArray(row1) ? row1[0] : row1?.x;
+ const rawX2 = Array.isArray(row2) ? row2[0] : row2?.x;
+
+ // If this chart's x-axis is temporal, coerce to timestamps (numbers) for
sorting.
+ // Fallback to original raw values if parsing fails.
+ const getComparableX = (raw: any) => {
+ if (xAxisType === AxisType.Time) {
+ // If it's already a number, use it. Otherwise try to coerce to Date
timestamp.
+ if (typeof raw === 'number' && isFinite(raw)) return raw;
+ const parsed = new Date(String(raw)).getTime();
+ return isFinite(parsed) ? parsed : String(raw);
+ }
+ return raw;
+ };
+
+ const x1 = getComparableX(rawX1);
+ const x2 = getComparableX(rawX2);
+
+ // naturalCompare already prefers numeric comparison when possible
+ return naturalCompare(x1, x2);
+ });
+});
Review Comment:
Using `(s as any)` type assertions multiple times (lines 398, 401) bypasses
TypeScript's type safety. Consider defining a proper interface or type for the
series data structure, or at least use a more specific type than `any` to
maintain type safety and code clarity.
```suggestion
// ----- ensure series data are sorted naturally on the x-value -----
// Run after all series have been created so each series.data is complete.
// Define a type for series with a data property
type SeriesWithData = SeriesOption & { data: any[] };
series.forEach((s: SeriesOption) => {
const dataArr = (s as SeriesWithData).data;
if (!Array.isArray(dataArr) || dataArr.length <= 1) return;
(s as SeriesWithData).data = dataArr.sort((row1: any, row2: any) => {
// extract the raw x values (support both [x,y] and { x, y } shapes)
const rawX1 = Array.isArray(row1) ? row1[0] : row1?.x;
const rawX2 = Array.isArray(row2) ? row2[0] : row2?.x;
// If this chart's x-axis is temporal, coerce to timestamps (numbers)
for sorting.
// Fallback to original raw values if parsing fails.
const getComparableX = (raw: any) => {
if (xAxisType === AxisType.Time) {
// If it's already a number, use it. Otherwise try to coerce to
Date timestamp.
if (typeof raw === 'number' && isFinite(raw)) return raw;
const parsed = new Date(String(raw)).getTime();
return isFinite(parsed) ? parsed : String(raw);
}
return raw;
};
const x1 = getComparableX(rawX1);
const x2 = getComparableX(rawX2);
// naturalCompare already prefers numeric comparison when possible
return naturalCompare(x1, x2);
});
});
```
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts:
##########
@@ -486,9 +537,7 @@ export default function transformProps(
const xAxisFormatter =
xAxisDataType === GenericDataType.Temporal
? getXAxisFormatter(xAxisTimeFormat)
- : xAxisDataType === GenericDataType.Numeric
- ? getNumberFormatter(xAxisNumberFormat)
- : String;
+ : String;
Review Comment:
The removal of numeric axis formatting (`xAxisNumberFormat` with
`getNumberFormatter`) means that numeric x-axes will now always use `String()`
for formatting, losing the ability to format numbers with thousands separators,
decimal precision, or other numeric formats. This is a breaking change that
could negatively affect charts with numeric x-axes (e.g., scatter plots with
numeric dimensions). Consider keeping the numeric formatter for
`GenericDataType.Numeric` x-axes.
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts:
##########
@@ -372,6 +391,38 @@ export default function transformProps(
}
}
});
+
+ // ----- ensure series data are sorted naturally on the x-value -----
+// Run after all series have been created so each series.data is complete.
+series.forEach((s: SeriesOption) => {
+ const dataArr = (s as any).data;
+ if (!Array.isArray(dataArr) || dataArr.length <= 1) return;
+
+ (s as any).data = dataArr.sort((row1: any, row2: any) => {
Review Comment:
The series data is being mutated in place with `Array.sort()`, which
modifies the original array. This could cause issues if the original data array
is referenced elsewhere. Consider creating a sorted copy instead: `(s as
any).data = [...dataArr].sort(...)` to avoid unintended side effects.
```suggestion
(s as any).data = [...dataArr].sort((row1: any, row2: any) => {
```
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts:
##########
@@ -110,6 +110,26 @@ import {
getYAxisFormatter,
} from '../utils/formatters';
+
+// ----- natural sort helper -----
+// Try numeric comparison first for numeric-like strings, fallback to
localeCompare.
+function naturalCompare(a: any, b: any): number {
+ const sa = a === undefined || a === null ? '' : String(a);
+ const sb = b === undefined || b === null ? '' : String(b);
+ const na = Number(sa);
+ const nb = Number(sb);
+
+ // If both parse as finite numbers, do numeric sort
+ if (isFinite(na) && isFinite(nb)) {
+ return na - nb;
+ }
Review Comment:
Converting empty strings to numbers will result in `0` (since `Number('')
=== 0`), which will pass the `isFinite()` check. This means null/undefined
values will be treated as `0` in numeric comparisons, causing them to sort
alongside actual zero values rather than being handled distinctly. This could
lead to incorrect ordering when the data contains both null values and zeros.
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts:
##########
@@ -110,6 +110,26 @@ import {
getYAxisFormatter,
} from '../utils/formatters';
+
+// ----- natural sort helper -----
+// Try numeric comparison first for numeric-like strings, fallback to
localeCompare.
+function naturalCompare(a: any, b: any): number {
+ const sa = a === undefined || a === null ? '' : String(a);
+ const sb = b === undefined || b === null ? '' : String(b);
+ const na = Number(sa);
+ const nb = Number(sb);
+
+ // If both parse as finite numbers, do numeric sort
+ if (isFinite(na) && isFinite(nb)) {
+ return na - nb;
+ }
+
+ // Otherwise fallback to lexicographic
+ return sa.localeCompare(sb);
+}
Review Comment:
The `naturalCompare` function will incorrectly sort empty strings before
numbers. When `a` or `b` is `null` or `undefined`, they are converted to empty
strings (`''`), which will then be compared using `localeCompare`. This means
null/undefined values will appear before numeric values, which may not be the
intended behavior. Consider handling null/undefined values explicitly by
placing them at the end of the sort order.
--
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]
