codeant-ai-for-open-source[bot] commented on code in PR #31765:
URL: https://github.com/apache/superset/pull/31765#discussion_r2597583372
##########
superset-frontend/plugins/plugin-chart-echarts/src/MixedTimeseries/transformProps.ts:
##########
@@ -543,11 +543,17 @@ export default function transformProps(
const tooltipFormatter =
xAxisDataType === GenericDataType.Temporal
- ? getTooltipTimeFormatter(tooltipTimeFormat)
+ ? getTooltipTimeFormatter(
+ tooltipTimeFormat,
+ formData.extraFormData.time_grain_sqla ?? timeGrainSqla,
+ )
: String;
const xAxisFormatter =
xAxisDataType === GenericDataType.Temporal
- ? getXAxisFormatter(xAxisTimeFormat)
+ ? getXAxisFormatter(
+ xAxisTimeFormat,
+ formData.extraFormData.time_grain_sqla ?? timeGrainSqla,
Review Comment:
**Suggestion:** Runtime error / wrong source: the same unsafe access to
`formData.extraFormData.time_grain_sqla` is used for the x-axis formatter; it
may throw if `extraFormData` is absent and should read from
`chartProps.rawFormData` to respect time-grain overrides — switch to
`chartProps.rawFormData?.extraFormData?.time_grain_sqla`. [possible bug]
**Severity Level:** Critical 🚨
```suggestion
chartProps.rawFormData?.extraFormData?.time_grain_sqla ??
timeGrainSqla,
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
Same issue as the tooltip formatter: accessing formData.extraFormData can
throw if extraFormData is undefined and won't pick up overrides from
rawFormData. Switching to
chartProps.rawFormData?.extraFormData?.time_grain_sqla both prevents the
TypeError and respects the intended override behavior.
</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/MixedTimeseries/transformProps.ts
**Line:** 555:555
**Comment:**
*Possible Bug: Runtime error / wrong source: the same unsafe access to
`formData.extraFormData.time_grain_sqla` is used for the x-axis formatter; it
may throw if `extraFormData` is absent and should read from
`chartProps.rawFormData` to respect time-grain overrides — switch to
`chartProps.rawFormData?.extraFormData?.time_grain_sqla`.
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:
##########
@@ -481,11 +480,17 @@ export default function transformProps(
const tooltipFormatter =
xAxisDataType === GenericDataType.Temporal
- ? getTooltipTimeFormatter(tooltipTimeFormat)
+ ? getTooltipTimeFormatter(
+ tooltipTimeFormat,
+ formData.extraFormData.time_grain_sqla ?? timeGrainSqla,
Review Comment:
**Suggestion:** Accessing `formData.extraFormData.time_grain_sqla` can throw
if `extraFormData` is undefined (property access on undefined). Also
`time_grain_sqla` belongs to the raw form data; use the raw form data and
optional chaining to safely read it and fall back to the existing
`timeGrainSqla` variable. [possible bug]
**Severity Level:** Critical 🚨
```suggestion
chartProps.rawFormData?.extraFormData?.time_grain_sqla ??
timeGrainSqla,
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
The existing code does access formData.extraFormData without any optional
chaining, which will throw if extraFormData is undefined at runtime. Using
optional chaining (or accessing rawFormData which always exists on chartProps)
is a safe, minimal fix that prevents a real runtime exception. This is a
functional correctness bug, not a stylistic 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/transformProps.ts
**Line:** 485:485
**Comment:**
*Possible Bug: Accessing `formData.extraFormData.time_grain_sqla` can
throw if `extraFormData` is undefined (property access on undefined). Also
`time_grain_sqla` belongs to the raw form data; use the raw form data and
optional chaining to safely read it and fall back to the existing
`timeGrainSqla` variable.
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/MixedTimeseries/transformProps.ts:
##########
@@ -543,11 +543,17 @@ export default function transformProps(
const tooltipFormatter =
xAxisDataType === GenericDataType.Temporal
- ? getTooltipTimeFormatter(tooltipTimeFormat)
+ ? getTooltipTimeFormatter(
+ tooltipTimeFormat,
+ formData.extraFormData.time_grain_sqla ?? timeGrainSqla,
Review Comment:
**Suggestion:** Runtime error / wrong source: accessing
`formData.extraFormData.time_grain_sqla` can throw when `extraFormData` is
undefined and also `extraFormData` lives on `rawFormData` (not `formData`) —
use `chartProps.rawFormData?.extraFormData?.time_grain_sqla` to both avoid a
TypeError and read the correct override. [null pointer]
**Severity Level:** Minor ⚠️
```suggestion
chartProps.rawFormData?.extraFormData?.time_grain_sqla ??
timeGrainSqla,
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
This is a valid bugfix: formData may not contain extraFormData and the
canonical place for user overrides is rawFormData. Using
chartProps.rawFormData?.extraFormData?.time_grain_sqla avoids a potential
TypeError and ensures we respect the original override. The suggested change
fixes a real runtime-null access and corrects the data source.
</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/MixedTimeseries/transformProps.ts
**Line:** 548:548
**Comment:**
*Null Pointer: Runtime error / wrong source: accessing
`formData.extraFormData.time_grain_sqla` can throw when `extraFormData` is
undefined and also `extraFormData` lives on `rawFormData` (not `formData`) —
use `chartProps.rawFormData?.extraFormData?.time_grain_sqla` to both avoid a
TypeError and read the correct override.
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/MixedTimeseries/transformProps.test.ts:
##########
@@ -16,8 +16,13 @@
* specific language governing permissions and limitations
* under the License.
*/
-import { ChartProps, VizType } from '@superset-ui/core';
import { supersetTheme } from '@apache-superset/core/ui';
+import {
+ ChartProps,
+ TimeFormatter,
+ TimeGranularity,
+ VizType,
+} from '@superset-ui/core';
Review Comment:
**Suggestion:** `TimeFormatter` is a type, not a runtime value; importing it
alongside runtime exports can lead to unnecessary runtime bindings or bundler
issues—import it using `import type` (or a separate type-only import) to avoid
emitting a value-level import. [type error]
**Severity Level:** Minor ⚠️
```suggestion
import { ChartProps, TimeGranularity, VizType } from '@superset-ui/core';
import type { TimeFormatter } from '@superset-ui/core';
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
This is a valid, useful suggestion: `TimeFormatter` is only used as a type
in the test and should be imported with `import type` to avoid accidentally
emitting a runtime import (especially important with Babel's TS handling). The
improved code fixes a real nicety and is 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/test/MixedTimeseries/transformProps.test.ts
**Line:** 20:25
**Comment:**
*Type Error: `TimeFormatter` is a type, not a runtime value; importing
it alongside runtime exports can lead to unnecessary runtime bindings or
bundler issues—import it using `import type` (or a separate type-only import)
to avoid emitting a value-level import.
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:
##########
@@ -481,11 +480,17 @@ export default function transformProps(
const tooltipFormatter =
xAxisDataType === GenericDataType.Temporal
- ? getTooltipTimeFormatter(tooltipTimeFormat)
+ ? getTooltipTimeFormatter(
+ tooltipTimeFormat,
+ formData.extraFormData.time_grain_sqla ?? timeGrainSqla,
+ )
: String;
const xAxisFormatter =
xAxisDataType === GenericDataType.Temporal
- ? getXAxisFormatter(xAxisTimeFormat)
+ ? getXAxisFormatter(
+ xAxisTimeFormat,
+ formData.extraFormData.time_grain_sqla ?? timeGrainSqla,
Review Comment:
**Suggestion:** The code reads `formData.extraFormData.time_grain_sqla`
which may not exist on `formData`; this will throw. Read the time grain from
the chart's raw form data with optional chaining so the expression never throws
and still falls back to `timeGrainSqla`. [possible bug]
**Severity Level:** Critical 🚨
```suggestion
chartProps.rawFormData?.extraFormData?.time_grain_sqla ??
timeGrainSqla,
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
Same issue as the tooltip formatter: accessing
formData.extraFormData.time_grain_sqla without optional chaining can throw if
extraFormData is undefined. Switching to safe access (e.g.,
chartProps.rawFormData?.extraFormData?.time_grain_sqla or
formData.extraFormData?.time_grain_sqla) prevents a real crash and is
appropriate here.
</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:** 492:492
**Comment:**
*Possible Bug: The code reads `formData.extraFormData.time_grain_sqla`
which may not exist on `formData`; this will throw. Read the time grain from
the chart's raw form data with optional chaining so the expression never throws
and still falls back to `timeGrainSqla`.
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/utils/formatters.ts:
##########
@@ -76,24 +77,40 @@ export const getYAxisFormatter = (
export function getTooltipTimeFormatter(
format?: string,
+ timeGrain?: TimeGranularity,
): TimeFormatter | StringConstructor {
+ if (
+ timeGrain === TimeGranularity.QUARTER ||
+ timeGrain === TimeGranularity.MONTH ||
+ timeGrain === TimeGranularity.YEAR
+ ) {
+ return getTimeFormatter(undefined, timeGrain);
+ }
if (format === SMART_DATE_ID) {
- return getSmartDateVerboseFormatter();
+ return getSmartDateVerboseFormatter(timeGrain);
Review Comment:
**Suggestion:** Logical ordering bug in `getTooltipTimeFormatter`: the new
early time-grain check runs before the `SMART_DATE_ID` check, so when callers
request `SMART_DATE_ID` with a QUARTER/MONTH/YEAR time grain the function will
return the generic time-grain formatter instead of the verbose smart-date
formatter. Reorder checks to honor explicit `SMART_DATE_ID` requests first.
[logic error]
**Severity Level:** Minor ⚠️
```suggestion
// If caller explicitly requested SMART_DATE_ID, respect that first
if (format === SMART_DATE_ID) {
return getSmartDateVerboseFormatter(timeGrain);
}
// Otherwise, if a coarse time grain is specified prefer the
grain-specific formatter
if (
timeGrain === TimeGranularity.QUARTER ||
timeGrain === TimeGranularity.MONTH ||
timeGrain === TimeGranularity.YEAR
) {
return getTimeFormatter(undefined, timeGrain);
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
This is a legitimate logic bug: an explicit request for SMART_DATE_ID should
be honored even when a coarse timeGrain is supplied. Reordering the checks so
SMART_DATE_ID is handled first preserves the caller's explicit intent. The
proposed change is minimal and local and fixes observable incorrect behavior.
</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/utils/formatters.ts
**Line:** 82:90
**Comment:**
*Logic Error: Logical ordering bug in `getTooltipTimeFormatter`: the
new early time-grain check runs before the `SMART_DATE_ID` check, so when
callers request `SMART_DATE_ID` with a QUARTER/MONTH/YEAR time grain the
function will return the generic time-grain formatter instead of the verbose
smart-date formatter. Reorder checks to honor explicit `SMART_DATE_ID` requests
first.
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]
