codeant-ai-for-open-source[bot] commented on code in PR #37794:
URL: https://github.com/apache/superset/pull/37794#discussion_r2804497453
##########
superset-frontend/plugins/plugin-chart-echarts/src/BigNumber/BigNumberWithTrendline/transformProps.ts:
##########
@@ -124,121 +129,75 @@ export default function transformProps(
detected_currency: detectedCurrency,
} = queriesData[0];
- const aggregatedQueryData = queriesData.length > 1 ? queriesData[1] : null;
-
- const hasAggregatedData =
- aggregatedQueryData?.data &&
- aggregatedQueryData.data.length > 0 &&
- aggregation !== 'LAST_VALUE';
-
- const aggregatedData = hasAggregatedData ? aggregatedQueryData.data[0] :
null;
const refs: Refs = {};
const metricName = getMetricLabel(metric);
const metrics = chartProps.datasource?.metrics || [];
const originalLabel = getOriginalLabel(metric, metrics);
const showMetricName = chartProps.rawFormData?.show_metric_name ?? false;
+
const compareLag = Number(compareLag_) || 0;
let formattedSubheader = subheader;
const { r, g, b } = colorPicker;
const mainColor = `rgb(${r}, ${g}, ${b})`;
const xAxisLabel = getXAxisLabel(rawFormData) as string;
+
let trendLineData: TimeSeriesDatum[] | undefined;
let percentChange = 0;
let bigNumber = data.length === 0 ? null : data[0][metricName];
let timestamp = data.length === 0 ? null : data[0][xAxisLabel];
let bigNumberFallback = null;
+
let sortedData: [number | null, number | null][] = [];
if (data.length > 0) {
sortedData = (data as BigNumberDatum[])
- .map(
- d =>
- [d[xAxisLabel], parseMetricValue(d[metricName])] as [
- number | null,
- number | null,
- ],
- )
- // sort in time descending order
- .sort((a, b) => (a[0] !== null && b[0] !== null ? b[0] - a[0] : 0));
+ .map(d => [d[xAxisLabel], parseMetricValue(d[metricName])])
+ .sort((a, b) => (a[0] && b[0] ? b[0] - a[0] : 0));
Review Comment:
**Suggestion:** The sort comparator for `sortedData` uses a truthiness check
on timestamps, which treats a valid timestamp of `0` (or other falsy numeric
values) as "missing" and skips proper ordering, potentially selecting the wrong
latest point for the big number, percent change, and trend line. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Big Number with Trendline may show wrong latest value.
- ⚠️ Percent change subheader can be computed from wrong point.
- ⚠️ Trendline ordering around epoch timestamp may be incorrect.
```
</details>
```suggestion
.sort((a, b) =>
a[0] !== null && b[0] !== null ? (b[0] as number) - (a[0] as number)
: 0,
);
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Create or use a dataset in Superset that has a temporal column containing
a row at the
Unix epoch (e.g., 1970‑01‑01 00:00:00), which is represented as timestamp
`0` in the
frontend data; use this column as the time axis in a "Big Number with
Trendline" chart, so
it is returned in `queriesData[0].data` and referenced by `xAxisLabel` in
`transformProps`
(`transformProps.ts:112–121`).
2. Ensure the dataset also has later timestamps (e.g., 1970‑01‑02,
1970‑01‑03, etc.) so
that `queriesData[0].data` includes both the `0` timestamp row and rows with
positive
timestamps when passed into `transformProps(chartProps)`
(`transformProps.ts:86`).
3. When the chart renders, `transformProps` builds `sortedData` at
`transformProps.ts:154–158` using `.sort((a, b) => (a[0] && b[0] ? b[0] -
a[0] : 0))`; for
any comparison where `a[0]` or `b[0]` is `0`, the `a[0] && b[0]` truthiness
check is
falsy, the comparator returns `0`, and the sort does not correctly order
that epoch row
relative to later timestamps.
4. The incorrectly ordered `sortedData` is then used to compute `timestamp`
and the
percent change (`sortedData[0]` and `sortedData[compareLag]`) and to build
`trendLineData`
(`transformProps.ts:160–191`), so the Big Number, percent change subheader,
and trendline
rendered for the "Big Number with Trendline" chart can reflect the wrong
"latest" data
point whenever a `0` timestamp is present.
```
</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/BigNumber/BigNumberWithTrendline/transformProps.ts
**Line:** 157:157
**Comment:**
*Logic Error: The sort comparator for `sortedData` uses a truthiness
check on timestamps, which treats a valid timestamp of `0` (or other falsy
numeric values) as "missing" and skips proper ordering, potentially selecting
the wrong latest point for the big number, percent change, and trend line.
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%2F37794&comment_hash=5a229050fe7b573a53c4f5a05ab9e7a84c7a4c73636888c1b39daa52a071ccb2&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F37794&comment_hash=5a229050fe7b573a53c4f5a05ab9e7a84c7a4c73636888c1b39daa52a071ccb2&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]
