codeant-ai-for-open-source[bot] commented on code in PR #36901:
URL: https://github.com/apache/superset/pull/36901#discussion_r2661018374
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/EchartsTimeseries.tsx:
##########
@@ -208,18 +209,47 @@ export default function EchartsTimeseries({
...(labelMap[seriesName] ?? []),
];
const groupBy = ensureIsArray(formData.groupby);
+ let xAxisDrillByFilter: BinaryQueryObjectFilterClause | undefined;
+
+ const xAxisColumn =
+ xAxis.label === DTTM_ALIAS
+ ? formData.granularitySqla
+ : xAxis.label;
+
if (data && xAxis.type === AxisType.Time) {
+
+ const timeValue = data[0];
+ const timeValueMs = typeof timeValue === 'number'
+ ? data[0]
+ : new Date(timeValue).getTime();
Review Comment:
**Suggestion:** The ternary that computes `timeValueMs` uses `data[0]` in
the true branch instead of the already-assigned `timeValue` variable; while
equivalent today, this is a logic clarity bug and can lead to future mistakes
if `timeValue` assignment changes — use `timeValue` directly. [logic error]
**Severity Level:** Minor ⚠️
```suggestion
const timeValueMs = typeof timeValue === 'number' ? timeValue :
new Date(timeValue).getTime();
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
This is a clarity / maintainability fix — functionally identical today, but
using the already-declared `timeValue` is clearer and avoids accidental future
bugs if the `timeValue` assignment changes. It's a harmless, readable
improvement.
</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:** 222:224
**Comment:**
*Logic Error: The ternary that computes `timeValueMs` uses `data[0]` in
the true branch instead of the already-assigned `timeValue` variable; while
equivalent today, this is a logic clarity bug and can lead to future mistakes
if `timeValue` assignment changes — use `timeValue` directly.
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:
##########
@@ -208,18 +209,47 @@ export default function EchartsTimeseries({
...(labelMap[seriesName] ?? []),
];
const groupBy = ensureIsArray(formData.groupby);
+ let xAxisDrillByFilter: BinaryQueryObjectFilterClause | undefined;
+
+ const xAxisColumn =
+ xAxis.label === DTTM_ALIAS
+ ? formData.granularitySqla
+ : xAxis.label;
+
if (data && xAxis.type === AxisType.Time) {
+
+ const timeValue = data[0];
+ const timeValueMs = typeof timeValue === 'number'
+ ? data[0]
+ : new Date(timeValue).getTime();
+ const grainMs =
+ TIMEGRAIN_TO_TIMESTAMP[formData.timeGrainSqla as keyof typeof
TIMEGRAIN_TO_TIMESTAMP] ??
+ TIMEGRAIN_TO_TIMESTAMP[TimeGranularity.DAY];
+ const startTime = new Date(timeValueMs).toISOString();
+ const endTime = new Date(timeValueMs + grainMs).toISOString();
+
drillToDetailFilters.push({
- col:
- // if the xAxis is '__timestamp', granularity_sqla will be the
column of filter
- xAxis.label === DTTM_ALIAS
- ? formData.granularitySqla
- : xAxis.label,
+ col: xAxisColumn,
grain: formData.timeGrainSqla,
op: '==',
- val: data[0],
- formattedVal: xValueFormatter(data[0]),
+ val: timeValue,
+ formattedVal: xValueFormatter(timeValue),
});
+ if (!Number.isNaN(timeValueMs)) {
Review Comment:
**Suggestion:** `new Date(timeValueMs).toISOString()` and `new
Date(timeValueMs + grainMs).toISOString()` are called before checking whether
`timeValueMs` is NaN; when `timeValueMs` is NaN this will throw a RangeError —
compute ISO strings only after verifying `timeValueMs` is a valid number.
[possible bug]
**Severity Level:** Critical 🚨
```suggestion
drillToDetailFilters.push({
col: xAxisColumn,
grain: formData.timeGrainSqla,
op: '==',
val: timeValue,
formattedVal: xValueFormatter(timeValue),
});
if (!Number.isNaN(timeValueMs)) {
const startTime = new Date(timeValueMs).toISOString();
const endTime = new Date(timeValueMs + grainMs).toISOString();
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
Constructing ISO strings with `new Date(timeValueMs).toISOString()` when
`timeValueMs` is NaN will throw a RangeError before the NaN check. The improved
code (compute start/end only after verifying `timeValueMs` is valid) fixes a
real crash scenario. This is a correct and necessary ordering change.
</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:** 228:238
**Comment:**
*Possible Bug: `new Date(timeValueMs).toISOString()` and `new
Date(timeValueMs + grainMs).toISOString()` are called before checking whether
`timeValueMs` is NaN; when `timeValueMs` is NaN this will throw a RangeError —
compute ISO strings only after verifying `timeValueMs` is a valid number.
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:
##########
@@ -208,18 +209,47 @@ export default function EchartsTimeseries({
...(labelMap[seriesName] ?? []),
];
const groupBy = ensureIsArray(formData.groupby);
+ let xAxisDrillByFilter: BinaryQueryObjectFilterClause | undefined;
+
+ const xAxisColumn =
+ xAxis.label === DTTM_ALIAS
+ ? formData.granularitySqla
+ : xAxis.label;
+
if (data && xAxis.type === AxisType.Time) {
+
+ const timeValue = data[0];
+ const timeValueMs = typeof timeValue === 'number'
+ ? data[0]
+ : new Date(timeValue).getTime();
+ const grainMs =
+ TIMEGRAIN_TO_TIMESTAMP[formData.timeGrainSqla as keyof typeof
TIMEGRAIN_TO_TIMESTAMP] ??
+ TIMEGRAIN_TO_TIMESTAMP[TimeGranularity.DAY];
+ const startTime = new Date(timeValueMs).toISOString();
+ const endTime = new Date(timeValueMs + grainMs).toISOString();
+
drillToDetailFilters.push({
- col:
- // if the xAxis is '__timestamp', granularity_sqla will be the
column of filter
- xAxis.label === DTTM_ALIAS
- ? formData.granularitySqla
- : xAxis.label,
+ col: xAxisColumn,
grain: formData.timeGrainSqla,
op: '==',
- val: data[0],
- formattedVal: xValueFormatter(data[0]),
+ val: timeValue,
+ formattedVal: xValueFormatter(timeValue),
});
+ if (!Number.isNaN(timeValueMs)) {
+ xAxisDrillByFilter = {
+ col: xAxisColumn,
+ op: 'TEMPORAL_RANGE',
+ val: `${startTime} : ${endTime}`,
+ formattedVal: xValueFormatter(timeValue),
+ };
+ }
+ } else {
+ xAxisDrillByFilter = {
+ col: xAxis.label,
+ op: '==',
+ val: data[0],
+ formattedVal: String(data[0]),
Review Comment:
**Suggestion:** The else branch unconditionally accesses `data[0]` even when
`data` can be null/undefined; this will throw a runtime TypeError when `data`
is falsy. Guard access to `data[0]` and avoid creating a filter when there is
no data point. [null pointer]
**Severity Level:** Minor ⚠️
```suggestion
} else if (data && data[0] !== undefined && data[0] !== null) {
xAxisDrillByFilter = {
col: xAxis.label,
op: '==',
val: data[0],
formattedVal: String(data[0]),
};
} else {
xAxisDrillByFilter = undefined;
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
The else branch in the current PR can run when `data` is falsy (the `if` is
`if (data && xAxis.type === AxisType.Time)`), so unguarded access to `data[0]`
is a real runtime risk. The suggested guard prevents a TypeError and is a
small, correct safety improvement. The change is low-risk and aligns with
surrounding checks that already test `data` elsewhere.
</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:** 246:251
**Comment:**
*Null Pointer: The else branch unconditionally accesses `data[0]` even
when `data` can be null/undefined; this will throw a runtime TypeError when
`data` is falsy. Guard access to `data[0]` and avoid creating a filter when
there is no data point.
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/src/components/Chart/DrillBy/DrillByModal.tsx:
##########
@@ -366,12 +366,35 @@ export default function DrillByModal({
updatedFormData.slice_id = 0;
delete updatedFormData.slice_name;
delete updatedFormData.dashboards;
+
+ if (
+ updatedFormData.viz_type == 'echarts_timeseries_bar' &&
Review Comment:
**Suggestion:** Use strict equality: the condition uses `==` to compare
`viz_type`, which can lead to unintended truthy coercions; use `===` for a
precise comparison to avoid subtle logic bugs. [logic error]
**Severity Level:** Minor ⚠️
```suggestion
updatedFormData.viz_type === 'echarts_timeseries_bar' &&
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
Using === is idiomatic and prevents accidental coercion bugs. There's no
reason to rely on JS loose-equality here — strict equality is safer and
functionally equivalent for this string comparison.
</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/DrillBy/DrillByModal.tsx
**Line:** 371:371
**Comment:**
*Logic Error: Use strict equality: the condition uses `==` to compare
`viz_type`, which can lead to unintended truthy coercions; use `===` for a
precise comparison to avoid subtle logic bugs.
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]
