codeant-ai-for-open-source[bot] commented on code in PR #41350:
URL: https://github.com/apache/superset/pull/41350#discussion_r3476833434
##########
superset-frontend/plugins/plugin-chart-echarts/src/MixedTimeseries/transformProps.ts:
##########
@@ -580,13 +580,19 @@ export default function transformProps(
if (maxSecondary === undefined) maxSecondary = 1;
}
+ // A dashboard-level time grain override (e.g. via a filter or the temporal
+ // range control) is delivered in extraFormData and should take precedence
+ // over the chart's own time grain when formatting temporal axes/tooltips.
+ const resolvedTimeGrain =
+ formData.extraFormData?.time_grain_sqla ?? timeGrainSqla;
+
const tooltipFormatter =
xAxisDataType === GenericDataType.Temporal
- ? getTooltipTimeFormatter(tooltipTimeFormat)
+ ? getTooltipTimeFormatter(tooltipTimeFormat, resolvedTimeGrain)
: String;
const xAxisFormatter =
xAxisDataType === GenericDataType.Temporal
- ? getXAxisFormatter(xAxisTimeFormat, timeGrainSqla)
+ ? getXAxisFormatter(xAxisTimeFormat, resolvedTimeGrain)
: String;
Review Comment:
**Suggestion:** This introduces a resolved dashboard grain for
formatter/interval behavior, but the max-label/dedup axis-label branch still
uses the chart-level grain variable, so mixed time-series can format ticks with
the override while still skipping the corresponding overlap protection. Apply
the resolved grain everywhere time-axis label behavior depends on grain to
avoid inconsistent tooltip/tick/label rendering. [incomplete implementation]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Mixed time-series dashboards show inconsistent x-axis label behavior.
- ⚠️ Tooltip and tick formats diverge from label overlap handling.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Render a Mixed Time-series ECharts chart, which uses `transformProps` from
`superset-frontend/plugins/plugin-chart-echarts/src/MixedTimeseries/transformProps.ts`
via
the export in
`superset-frontend/plugins/plugin-chart-echarts/src/index.ts:52` (`export {
default as MixedTimeseriesTransformProps } from
'./MixedTimeseries/transformProps';`).
2. Configure the chart so that `xAxisDataType` is temporal and `xAxisType`
is time
(standard MixedTimeseries time-series configuration), but leave the
chart-level
`timeGrainSqla` unset in form data while a dashboard-level filter or time
range control
supplies `extra_form_data.time_grain_sqla` for the same chart (this populates
`formData.extraFormData.time_grain_sqla` but not `timeGrainSqla` when
`transformProps` is
called).
3. When `transformProps` executes, at lines 586–596 the code sets
`resolvedTimeGrain =
formData.extraFormData?.time_grain_sqla ?? timeGrainSqla` and passes it into
`getTooltipTimeFormatter`/`getXAxisFormatter`, so tooltip and axis tick
formats, as well
as `minInterval`/`maxInterval` on the x-axis (lines 149–160), all respect
the dashboard
override even though `timeGrainSqla` itself is still undefined.
4. Immediately after, the max-label/dedup logic (around lines 39–60 in the
same file)
computes `showMaxLabel` as `xAxisType === AxisType.Time &&
xAxisLabelRotation === 0 &&
!!timeGrainSqla`, and only wraps the formatter when `showMaxLabel` is true;
in the
scenario above `!!timeGrainSqla` is false even though `resolvedTimeGrain` is
set, so
`showMaxLabel` stays false and `deduplicatedFormatter` falls back to the raw
`xAxisFormatter`, meaning the dashboard override controls tooltip/tick
formatting but not
the max-label/overlap-protection behavior, leading to inconsistent axis
labeling for Mixed
Timeseries when grain comes solely from `extra_form_data`.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=a60db4b1e8f546d0b28b67432ef837e3&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=a60db4b1e8f546d0b28b67432ef837e3&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:** 586:596
**Comment:**
*Incomplete Implementation: This introduces a resolved dashboard grain
for formatter/interval behavior, but the max-label/dedup axis-label branch
still uses the chart-level grain variable, so mixed time-series can format
ticks with the override while still skipping the corresponding overlap
protection. Apply the resolved grain everywhere time-axis label behavior
depends on grain to avoid inconsistent tooltip/tick/label rendering.
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=fe9e9be296adf4f53ff8f4351b96b0a805110e6c92947e09ab033b6618c5ed6a&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41350&comment_hash=fe9e9be296adf4f53ff8f4351b96b0a805110e6c92947e09ab033b6618c5ed6a&reaction=dislike'>👎</a>
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts:
##########
@@ -724,13 +724,19 @@ export default function transformProps(
}
}
+ // A dashboard-level time grain override (e.g. via a filter or the temporal
+ // range control) is delivered in extraFormData and should take precedence
+ // over the chart's own time grain when formatting temporal axes/tooltips.
+ const resolvedTimeGrain =
Review Comment:
**Suggestion:** The dashboard override is only wired into formatter/interval
selection, but the label-dedup/last-label path still keys off the original
chart grain (`timeGrainSqla`). When a dashboard-level grain is applied to a
chart without its own grain, axis formatting uses the override while max-label
handling does not, which can reintroduce duplicated or hidden terminal time
labels. Use the same resolved grain consistently for all time-axis label logic.
[incomplete implementation]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Time-series charts may hide last timestamp label with overrides.
- ⚠️ Users can misread trends when terminal labels are inconsistent.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Render a Time-series ECharts chart, which uses `transformProps` from
`superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts`
via the
export in `superset-frontend/plugins/plugin-chart-echarts/src/index.ts:55`
(`export {
default as TimeseriesTransformProps } from './Timeseries/transformProps';`).
2. Configure the chart so that `xAxisDataType` is temporal and `xAxisType`
is time with
`xAxisLabelRotation === 0`, but do not set a chart-level time grain (so
`timeGrainSqla` is
undefined) while a dashboard-level filter or time control supplies
`extra_form_data.time_grain_sqla` for that chart, causing
`formData.extraFormData.time_grain_sqla` to be defined when `transformProps`
is invoked.
3. In `transformProps` at lines 730–743, `resolvedTimeGrain =
formData.extraFormData?.time_grain_sqla ?? timeGrainSqla` is computed and
then passed into
`getTooltipTimeFormatter` and `getXAxisFormatter`, and later used for
`minInterval`/`maxInterval` (lines 232–243), so tooltip and axis tick
formatting fully
honor the dashboard override even though the original `timeGrainSqla`
remains unset.
4. Further down, the label-dedup/max-label code (lines 176–201) sets
`showMaxLabel =
xAxisType === AxisType.Time && xAxisLabelRotation === 0 && !!timeGrainSqla`
and only
applies the `deduplicatedFormatter` and `axisLabel.showMaxLabel` when this
boolean is
true; in the override-only scenario `!!timeGrainSqla` is false despite
`resolvedTimeGrain`
being set, so the logic intended to keep the last time label visible and
deduplicate axis
boundary labels (per the comment at lines 176–179/217–225) is skipped,
reintroducing the
possibility of duplicated or hidden terminal labels when the time grain is
provided via
`extra_form_data` instead of the chart-level control.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=dbe0b48f27fc499686a40679ce11f22f&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=dbe0b48f27fc499686a40679ce11f22f&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:** 730:743
**Comment:**
*Incomplete Implementation: The dashboard override is only wired into
formatter/interval selection, but the label-dedup/last-label path still keys
off the original chart grain (`timeGrainSqla`). When a dashboard-level grain is
applied to a chart without its own grain, axis formatting uses the override
while max-label handling does not, which can reintroduce duplicated or hidden
terminal time labels. Use the same resolved grain consistently for all
time-axis label logic.
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=480c2bc351af19557b09203bc4d43dc6f5f70a9fa7843ec8cef3e4b1c8699425&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41350&comment_hash=480c2bc351af19557b09203bc4d43dc6f5f70a9fa7843ec8cef3e4b1c8699425&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]
