EnxDev commented on PR #42299: URL: https://github.com/apache/superset/pull/42299#issuecomment-5339679818
## EnxDev's Review Agent โ apache/superset#42299 ยท HEAD c90cb13c **comment** โ the root cause is correctly identified and the regression guards are real, but `hideOverlap: false` lands on the *default* chart config, and the `interval: 'auto'` argument used to dismiss that on both threads doesn't hold for `type: 'time'` axes. Verified the premise first: in echarts 6.1.0 `AxisBuilder.layOutAxisTickLabel` calls `fixMinMaxLabelShow` and *then* `hideOverlap`, so a forced max label can indeed be dropped after being forced. The bug in #39899 is real and this diff targets the actual mechanism. ### ๐ด Functional - **`superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts:1259`** (same at **`MixedTimeseries/transformProps.ts:770`**) ยท _Medium_ โ `showMaxLabel` is not a narrow case: it's `Time axis && rotation === 0 && grain`, and `xAxisLabelRotation` defaults to `0` (`src/defaults.ts`), so every temporal chart with a resolved grain โ the default configuration โ loses label-collision handling. The counter-argument on both threads (`interval: 'auto'` still thins ticks) doesn't apply to this axis type: echarts routes non-category axes through `makeRealNumberLabels`, which never reads `axisLabel.interval` (`coord/axisTickLabelBuilder.js`, `createAxisLabels` branches on `axis.type === 'category'`). Time-axis tick count is `span / splitNumber` (`scale/Time.js`, `calcNiceForTimeScale`) and is width-independent, and Superset sets `splitNumber` only on the yAxis (`transformProps.ts:1317`). Echarts' own comment at the `hideOverlap` call site โ "This bit fixes the la bel overlap issue for the time chart" (echarts#14266) โ is about exactly this case. Net effect: a narrow dashboard tile with a day grain renders ~6-10 date labels into ~300px with nothing thinning them. The yAxis already has the right pattern here (`splitNumber` tiered off available pixels) โ deriving an xAxis `splitNumber` from `width`, or gating the disable on width, fixes the boundary label without dropping collision handling globally. **regression test:** transform at `width: 300` with a day grain and assert the guard survives (`hideOverlap` true, or a reduced xAxis `splitNumber`); today nothing fails when the axis goes unthinned. One mitigation worth knowing, since it narrows what the diff actually buys: `fixMinMaxLabelShow` already hides the *neighbour* of a forced min/max label when they intersect, so the boundary collision was handled. What the disable adds is protection against the max label losing to a non-adjacent label โ paid for with every intermediate collision. ### ๐ก Should-fix - **`test/Timeseries/transformers.test.ts:351`** โ flipping this assertion removes the last test that pinned `hideOverlap: true` on a timeseries x-axis; the surviving `!(Time && rotation !== 0)` branch is now unpinned repo-wide. Add a case for time axis + 0ยฐ rotation + no time grain โ `hideOverlap` true, otherwise the next edit collapses the ternary silently. ### ๐ต Nits - `test/MixedTimeseries/transformProps.test.ts:1167` โ the same two-row fixture is constructed four times (`defaultQueriesData` ร2, `queriesData` ร2); hoist it to one const. - `test/Timeseries/transformers.test.ts:343` โ the name still claims "x-axis dates do not overlap" while the body now asserts overlap detection is off. Rename to what it guards. - `MixedTimeseries/transformProps.ts:770` โ the Timeseries counterpart carries the rationale comment; this one has none. ### ๐ Praise - `test/MixedTimeseries/transformProps.test.ts:1167` โ it genuinely fails without the fix (the old value at 0ยฐ rotation was `true`), and the note about needing `timeGrainSqla` set to actually reach the branch is what keeps a test from silently no-opping. Addresses the review request for mixed coverage. CI is green apart from the netlify docs preview (not run). Title is conventional-commits clean; no migration, auth, or SQL surface. <!-- enxdev-review-agent:c90cb13c --> _Reviewed by EnxDev's Review Agent โ @EnxDev ยท HEAD c90cb13c._ -- 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]
