codeant-ai-for-open-source[bot] commented on code in PR #41350: URL: https://github.com/apache/superset/pull/41350#discussion_r3477136334
########## superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts: ########## @@ -871,7 +877,9 @@ export default function transformProps( // "2005" appears twice with Year grain). Wrap the formatter to suppress // consecutive duplicate labels. const showMaxLabel = - xAxisType === AxisType.Time && xAxisLabelRotation === 0 && !!timeGrainSqla; + xAxisType === AxisType.Time && + xAxisLabelRotation === 0 && + !!resolvedTimeGrain; const deduplicatedFormatter = showMaxLabel Review Comment: **Suggestion:** `showMaxLabel` is now conditioned on `resolvedTimeGrain`, which disables the "always show last tick label" behavior for time axes when no grain is set. On non-rotated temporal charts without a grain, `hideOverlap` can hide the final label again, regressing the earlier fix for missing last labels. Keep `showMaxLabel` tied to time-axis/rotation, and gate only the duplicate-suppression logic on time grain. [logic error] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ⚠️ Timeseries x-axes may hide last timestamp label. - ⚠️ Dashboards using VizType.Timeseries show incomplete end label. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. A generic time-series visualization (e.g. `VizType.Timeseries`) is registered in `src/visualizations/presets/MainPreset.ts:12-14` using `EchartsTimeseriesChartPlugin` from `plugins/plugin-chart-echarts/src/Timeseries/index.ts:34-75`, which wires `transformProps` from `plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts`. 2. Time-series form data is based on `EchartsTimeseriesFormData` and `DEFAULT_FORM_DATA` (`Timeseries/constants.ts:36-91`), which does not set `timeGrainSqla`, and tests for the chart component explicitly use `time_grain_sqla: undefined` in `Timeseries/EchartsTimeseries.test.tsx:5-19`, so for many charts both `timeGrainSqla` and `extraFormData?.time_grain_sqla` can be `undefined`. 3. In `Timeseries/transformProps.ts:731`, `resolvedTimeGrain` is computed as `formData.extraFormData?.time_grain_sqla ?? timeGrainSqla`, so with the form data above `resolvedTimeGrain === undefined`; with a temporal x-axis (`xAxisDataType` temporal leading to `xAxisType = getAxisType(...)` at `Timeseries/transformProps.ts:317-13`) and default `xAxisLabelRotation = 0` (from `defaults.ts:30-33` and `Timeseries/constants.ts:82`), the `showMaxLabel` flag at the hunk in `Timeseries/transformProps.ts` lines 880-883 evaluates to `false` solely because `!!resolvedTimeGrain` is false. 4. The x-axis configuration built at `Timeseries/transformProps.ts:241-251` sets `axisLabel.hideOverlap: true` and only conditionally adds `showMaxLabel: true` when `showMaxLabel` is truthy; with `showMaxLabel` now gated on `resolvedTimeGrain`, non-rotated time axes without an active grain lose the forced-max-label behavior that was added to prevent `hideOverlap` from hiding the last tick label (as described in the comment at `Timeseries/transformProps.ts:235-247`), so the final timestamp label can disappear again in those charts. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=5b5397fd73b14b3a95ae0f010943e6c5&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=5b5397fd73b14b3a95ae0f010943e6c5&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:** 880:883 **Comment:** *Logic Error: `showMaxLabel` is now conditioned on `resolvedTimeGrain`, which disables the "always show last tick label" behavior for time axes when no grain is set. On non-rotated temporal charts without a grain, `hideOverlap` can hide the final label again, regressing the earlier fix for missing last labels. Keep `showMaxLabel` tied to time-axis/rotation, and gate only the duplicate-suppression logic on time grain. 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=71390d464a018936a3fd4493ca9a2838f1c6631702fbba97519757881c675ca1&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41350&comment_hash=71390d464a018936a3fd4493ca9a2838f1c6631702fbba97519757881c675ca1&reaction=dislike'>👎</a> ########## superset-frontend/plugins/plugin-chart-echarts/src/MixedTimeseries/transformProps.ts: ########## @@ -580,17 +580,25 @@ 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; const showMaxLabel = - xAxisType === AxisType.Time && xAxisLabelRotation === 0 && !!timeGrainSqla; + xAxisType === AxisType.Time && + xAxisLabelRotation === 0 && + !!resolvedTimeGrain; Review Comment: **Suggestion:** This introduces the same regression as in Timeseries: `showMaxLabel` now requires `resolvedTimeGrain`, so mixed temporal charts with no active grain no longer force-render the last tick label. That allows `hideOverlap` to suppress the final label on non-rotated time axes. Preserve max-label forcing independently of grain and only use grain to decide whether deduplication is needed. [logic error] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ⚠️ Mixed chart x-axes may hide last time label. - ⚠️ Dashboards using mixed_timeseries misrepresent visible time coverage. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. A Mixed Chart visualization (`VizType.MixedTimeseries`) uses `EchartsMixedTimeseriesChartPlugin`, which wires `transformProps` from `plugins/plugin-chart-echarts/src/MixedTimeseries/transformProps.ts` via the plugin export in `plugins/plugin-chart-echarts/src/index.ts:1-8` and the plugin class in `MixedTimeseries/index.ts:19-37`. 2. Mixed chart form data is typed as `EchartsMixedTimeseriesFormData` (`MixedTimeseries/types.ts:44-94`) and defaults to `DEFAULT_FORM_DATA` (`MixedTimeseries/types.ts:96-142), which does not set `timeGrainSqla`, and typical requests also omit `extraFormData.time_grain_sqla`, so `resolvedTimeGrain` in `MixedTimeseries/transformProps.ts:583-587` becomes `undefined`. 3. For a standard temporal x-axis (e.g. the `ds` timestamp column used in `MixedTimeseries/transformProps.test.ts:121-130`), `getAxisType` (earlier in `MixedTimeseries/transformProps.ts`) produces `xAxisType === AxisType.Time`, and `xAxisLabelRotation` defaults to 0 via `defaultXAxis.xAxisLabelRotation` (`plugins/plugin-chart-echarts/src/defaults.ts:30-33`) and `DEFAULT_FORM_DATA` (`MixedTimeseries/types.ts:139-140`). 4. Under these conditions, the `showMaxLabel` flag defined at the hunk in `MixedTimeseries/transformProps.ts` lines 598-601 evaluates to `false` because `!!resolvedTimeGrain` is false, so the x-axis config at `MixedTimeseries/transformProps.ts:135-148` sets `hideOverlap: true` without `showMaxLabel: true`, allowing ECharts' overlap logic to hide the final tick label on mixed time-series charts when no time grain is active, regressing the intended "always show last label" behavior. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=f2d9f1220f184cacb54cf9e8d6cf0136&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=f2d9f1220f184cacb54cf9e8d6cf0136&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:** 598:601 **Comment:** *Logic Error: This introduces the same regression as in Timeseries: `showMaxLabel` now requires `resolvedTimeGrain`, so mixed temporal charts with no active grain no longer force-render the last tick label. That allows `hideOverlap` to suppress the final label on non-rotated time axes. Preserve max-label forcing independently of grain and only use grain to decide whether deduplication is needed. 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=d1f36fcf671c5b80e1178968ae2c85fb11cf6db4bf708001c43a20001756447d&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41350&comment_hash=d1f36fcf671c5b80e1178968ae2c85fb11cf6db4bf708001c43a20001756447d&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]
