codeant-ai-for-open-source[bot] commented on code in PR #41350:
URL: https://github.com/apache/superset/pull/41350#discussion_r3471369059
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts:
##########
@@ -916,13 +922,13 @@ export default function transformProps(
minInterval:
xAxisType === AxisType.Time && timeGrainSqla && !forceMaxInterval
? TIMEGRAIN_TO_TIMESTAMP[
- timeGrainSqla as keyof typeof TIMEGRAIN_TO_TIMESTAMP
+ resolvedTimeGrain as keyof typeof TIMEGRAIN_TO_TIMESTAMP
]
: 0,
maxInterval:
xAxisType === AxisType.Time && timeGrainSqla && forceMaxInterval
? TIMEGRAIN_TO_TIMESTAMP[
- timeGrainSqla as keyof typeof TIMEGRAIN_TO_TIMESTAMP
+ resolvedTimeGrain as keyof typeof TIMEGRAIN_TO_TIMESTAMP
]
: undefined,
Review Comment:
**Suggestion:** The interval lookup now uses `resolvedTimeGrain`, but the
guard still checks `timeGrainSqla`, so dashboard-level grain overrides are not
consistently applied to axis interval behavior. This causes tooltip/x-axis
formatting to use the override while interval constraints may stay disabled or
resolve to `undefined`, leading to inconsistent tick density and label
behavior. Use the resolved grain for both the guard and lookup, and add a safe
fallback when the grain is not present in `TIMEGRAIN_TO_TIMESTAMP`. [logic
error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Time-series x-axis intervals ignore dashboard time-grain overrides.
- ⚠️ Users see grain-based labels but unconstrained tick spacing.
- ⚠️ Unmapped override grains silently disable interval constraints.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Render a standard ECharts Time-series chart so that
`EchartsTimeseriesChartPlugin` in
`superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/index.ts:5-13`
is used; it
wires `transformProps` from
`superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts`
as the
transform function.
2. As with the mixed chart, `EchartsTimeseriesFormData` extends
`QueryFormData` and has an
optional `timeGrainSqla?: TimeGranularity` field
(`superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/types.ts:49-50`).
In
`transformProps`, `formData` is destructured from `chartProps` at
`superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts:102-118`,
and `timeGrainSqla` is one of the fields pulled from `formData` (verified by
the Grep hit
at this path and its later uses).
3. Apply a dashboard-level time grain override so that
`formData.extraFormData.time_grain_sqla` is populated while leaving the
chart's own
`timeGrainSqla` unset (or using a different grain). The override is
explicitly consumed in
`transformProps` at
`superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts:39-43`
(around line 721: `const resolvedTimeGrain =
formData.extraFormData?.time_grain_sqla ??
timeGrainSqla;`), and `resolvedTimeGrain` is then used for tooltip and
x-axis formatting
at lines 45-52 (`getTooltipTimeFormatter(tooltipTimeFormat,
resolvedTimeGrain)` and
`getXAxisFormatter(xAxisTimeFormat, resolvedTimeGrain)`).
4. Inspect the x-axis config built later in the same function, at
`superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts:214-254`
(lines 243-254 in the BulkRead snippet corresponding to PR lines 922-933):
`minInterval`
is computed as `xAxisType === AxisType.Time && timeGrainSqla &&
!forceMaxInterval ?
TIMEGRAIN_TO_TIMESTAMP[resolvedTimeGrain as keyof typeof
TIMEGRAIN_TO_TIMESTAMP] : 0`, and
`maxInterval` similarly uses `timeGrainSqla` in the guard but
`resolvedTimeGrain` in the
lookup. When `xAxisType` is `AxisType.Time` but the chart-level
`timeGrainSqla` is
undefined and only the dashboard override is set, the condition
short-circuits and
`TIMEGRAIN_TO_TIMESTAMP` is never consulted, so the axis interval does not
reflect the
dashboard grain even though tooltips and labels do. Conversely, when
`timeGrainSqla` is
set but `resolvedTimeGrain` is overridden to a different or unmapped
`TimeGranularity`,
the guard passes but `TIMEGRAIN_TO_TIMESTAMP[resolvedTimeGrain]` (from
`superset-frontend/plugins/plugin-chart-echarts/src/constants.ts:92-98`) can
yield
`undefined`, causing `minInterval`/`maxInterval` to be effectively disabled.
In both
cases, the axis uses inconsistent tick spacing relative to the grain implied
by
tooltip/x-axis formatting.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=35885e3678a34028ba630d1efcd9aec6&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=35885e3678a34028ba630d1efcd9aec6&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/transformProps.ts
**Line:** 922:933
**Comment:**
*Logic Error: The interval lookup now uses `resolvedTimeGrain`, but the
guard still checks `timeGrainSqla`, so dashboard-level grain overrides are not
consistently applied to axis interval behavior. This causes tooltip/x-axis
formatting to use the override while interval constraints may stay disabled or
resolve to `undefined`, leading to inconsistent tick density and label
behavior. Use the resolved grain for both the guard and lookup, and add a safe
fallback when the grain is not present in `TIMEGRAIN_TO_TIMESTAMP`.
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%2F41350&comment_hash=9a26d684c728a7cb06d6c7c7412a7ec0f06488593122d06fbc8f6f2007493f1e&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41350&comment_hash=9a26d684c728a7cb06d6c7c7412a7ec0f06488593122d06fbc8f6f2007493f1e&reaction=dislike'>👎</a>
##########
superset-frontend/plugins/plugin-chart-echarts/src/MixedTimeseries/transformProps.ts:
##########
@@ -702,13 +708,13 @@ export default function transformProps(
minInterval:
xAxisType === AxisType.Time && timeGrainSqla && !forceMaxInterval
? TIMEGRAIN_TO_TIMESTAMP[
- timeGrainSqla as keyof typeof TIMEGRAIN_TO_TIMESTAMP
+ resolvedTimeGrain as keyof typeof TIMEGRAIN_TO_TIMESTAMP
]
: 0,
maxInterval:
xAxisType === AxisType.Time && timeGrainSqla && forceMaxInterval
? TIMEGRAIN_TO_TIMESTAMP[
- timeGrainSqla as keyof typeof TIMEGRAIN_TO_TIMESTAMP
+ resolvedTimeGrain as keyof typeof TIMEGRAIN_TO_TIMESTAMP
]
: undefined,
...getMinAndMaxFromBounds(
Review Comment:
**Suggestion:** This interval calculation has the same mismatch between
guard and lookup source: it checks `timeGrainSqla` but indexes with
`resolvedTimeGrain`. When a dashboard override is present, interval handling
can diverge from tooltip/x-axis formatting and return `undefined` for unmapped
grains, resulting in inconsistent temporal axis rendering. Switch the guard to
the resolved grain and defensively handle missing map entries. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Mixed timeseries x-axis ticks ignore dashboard grain overrides.
- ⚠️ Tooltip/x-axis labels use grain while intervals stay default.
- ⚠️ Unmapped grains yield undefined intervals despite grain being set.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Render a Mixed Time-series chart so that `EchartsTimeseriesChartPlugin` in
`superset-frontend/plugins/plugin-chart-echarts/src/MixedTimeseries/index.ts:34-57`
is
used, which wires `transformProps` from
`superset-frontend/plugins/plugin-chart-echarts/src/MixedTimeseries/transformProps.ts`
as
the chart's transform function.
2. Ensure the chart's own time grain is unset while the chart still uses a
temporal
x-axis: `timeGrainSqla?: TimeGranularity` is optional on
`EchartsMixedTimeseriesFormData`
(`superset-frontend/plugins/plugin-chart-echarts/src/MixedTimeseries/types.ts:20`),
and it
is destructured (along with `forceMaxInterval`) from `formData` in
`transformProps` at
`superset-frontend/plugins/plugin-chart-echarts/src/MixedTimeseries/transformProps.ts:200-30`
(line 223 is `timeGrainSqla,`), while `xAxisType` is computed from column
types at lines
60-70 in the same file, so `xAxisType` can still be `AxisType.Time` even when
`timeGrainSqla` is undefined.
3. Apply a dashboard-level time grain override (e.g. from a native filter or
temporal
range control) so that `formData.extraFormData.time_grain_sqla` is populated
— the code in
this PR explicitly consumes this override in `transformProps` at
`superset-frontend/plugins/plugin-chart-echarts/src/MixedTimeseries/transformProps.ts:44-48`
(around line 587: `const resolvedTimeGrain =
formData.extraFormData?.time_grain_sqla ??
timeGrainSqla;`), and uses `resolvedTimeGrain` for tooltip and x-axis
formatting at lines
50-57 (`getTooltipTimeFormatter(tooltipTimeFormat, resolvedTimeGrain)` and
`getXAxisFormatter(xAxisTimeFormat, resolvedTimeGrain)`).
4. Observe the x-axis interval configuration at
`superset-frontend/plugins/plugin-chart-echarts/src/MixedTimeseries/transformProps.ts:169-180`
(lines 709-720 in the PR): `minInterval` and `maxInterval` are guarded by
`xAxisType ===
AxisType.Time && timeGrainSqla && !forceMaxInterval` / `&&
forceMaxInterval`, but the
lookup uses `TIMEGRAIN_TO_TIMESTAMP[resolvedTimeGrain as keyof typeof
TIMEGRAIN_TO_TIMESTAMP]`. When `timeGrainSqla` is undefined but
`resolvedTimeGrain` is set
from `extraFormData.time_grain_sqla`, the guard evaluates to false so the
ternary falls
through to `0` / `undefined` and the mapping is never used. Tooltips and
x-axis labels are
still formatted using the dashboard override, but the underlying ECharts
`minInterval`/`maxInterval` do not reflect that override, producing
inconsistent tick
spacing versus the grain shown in tooltips; additionally, if
`resolvedTimeGrain` is a
valid `TimeGranularity` that is not present in `TIMEGRAIN_TO_TIMESTAMP`
(defined in
`superset-frontend/plugins/plugin-chart-echarts/src/constants.ts:92-98`),
the guard can
pass (when `timeGrainSqla` is truthy) but the lookup yields `undefined`,
silently
disabling the intended interval constraint.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=8a2fdaf5eda44847b7343942081a937e&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=8a2fdaf5eda44847b7343942081a937e&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/MixedTimeseries/transformProps.ts
**Line:** 709:720
**Comment:**
*Logic Error: This interval calculation has the same mismatch between
guard and lookup source: it checks `timeGrainSqla` but indexes with
`resolvedTimeGrain`. When a dashboard override is present, interval handling
can diverge from tooltip/x-axis formatting and return `undefined` for unmapped
grains, resulting in inconsistent temporal axis rendering. Switch the guard to
the resolved grain and defensively handle missing map entries.
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%2F41350&comment_hash=aa58ad5fc08e41a79b3995511b34ea1010aac1034277ebdb7729fc861b6f2afe&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41350&comment_hash=aa58ad5fc08e41a79b3995511b34ea1010aac1034277ebdb7729fc861b6f2afe&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]
