You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "bikash-barnwal (via GitHub)" <gi...@apache.org> on 2026/08/14 21:32:05 UTC
[PR] fix(echarts): honor "All" for the x-axis label interval [superset]
bikash-barnwal opened a new pull request, #43193:
URL: https://github.com/apache/superset/pull/43193
### SUMMARY
Setting **X Axis Label Interval** to *All* did nothing — charts kept showing every nth label.
The control is a `SelectControl` whose choices are `['auto', 'Auto']` and `['0', 'All']`, so *All* is stored as the **string** `'0'`. ECharts' `axisLabel.interval` only understands `'auto'` or a number; a string is not a valid value, so it was ignored and the axis fell back to its default label thinning.
There is a second half to it. Even with a numeric `0`, `hideOverlap` is enabled for non-rotated axes:
```ts
hideOverlap: !(xAxisType === AxisType.Time && xAxisLabelRotation !== 0),
```
and `hideOverlap` drops labels that collide — exactly the labels *All* exists to show. So fixing only the type coercion would still not honor the setting.
This adds a small `getAxisLabelInterval` helper that normalizes the control value to what ECharts accepts (falling back to `'auto'` for anything unparseable, rather than handing the axis a bad value) and reports whether the user asked for every label. Timeseries and MixedTimeseries use it to set `interval` and to disable `hideOverlap` in that one case.
**Left alone deliberately:** Bubble passes the same control value to `xAxis.interval` rather than `axisLabel.interval` — a different option, measured in value units rather than label counts. Changing it would alter tick spacing, not label density, so it is out of scope here and worth a separate look.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Before: with *All* selected, an Area/Time-series chart still renders roughly every 4th label.
After: every category label renders.
### TESTING INSTRUCTIONS
```bash
cd superset-frontend
npm run test -- plugins/plugin-chart-echarts/test
```
**775 tests across 69 suites pass.** New cases: a unit test for the helper (string `'0'`, `'auto'`, `undefined`, `null`, a nonsense value, and an explicit `2`), and a Timeseries test asserting `interval === 0` with `hideOverlap === false`. Two of the three fail on unpatched source; the third pins the `'auto'` default so the new branch cannot regress it.
Manually: any Time-series or Area chart → Customize → **X Axis Label Interval** → *All*, and confirm every label renders.
### ADDITIONAL INFORMATION
- [x] Has associated issue: Fixes #36325
- [ ] Required feature flags:
- [x] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
🤖 Generated with [Claude Code](https://claude.com/claude-code)
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(echarts): honor "All" for the x-axis label interval [superset]
Posted by "EnxDev (via GitHub)" <gi...@apache.org>.
EnxDev commented on code in PR #43193:
URL: https://github.com/apache/superset/pull/43193#discussion_r3966288186
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts:
##########
@@ -1204,6 +1205,26 @@ export default function transformProps(
xAxisType === AxisType.Time &&
xAxisLabelRotation === 0 &&
!!resolvedTimeGrain;
+ const {
+ interval: xAxisLabelIntervalValue,
+ showAllLabels: showAllXAxisLabels,
+ } = getAxisLabelInterval(xAxisLabelInterval);
+ // axisLabel.interval is only ever consulted for category axes in ECharts
+ // (axisTickLabelBuilder routes to makeCategoryLabels there; a time axis
+ // goes through makeRealNumberLabels and never reads it), and Superset sets
+ // xAxis.type to Time whenever the x-axis column is temporal -- the common
+ // case #36325 actually reports. On a time axis, tick density is governed
+ // by minInterval/maxInterval instead, and minInterval alone only floors
+ // the spacing: ECharts can still choose a wider "nice" interval to fit the
+ // available width. Pinning both bounds to the resolved grain forces one
+ // tick, and therefore one label, per data point.
+ const timeGrainIntervalMs = resolvedTimeGrain
+ ? (TIMEGRAIN_TO_TIMESTAMP[
+ resolvedTimeGrain as keyof typeof TIMEGRAIN_TO_TIMESTAMP
+ ] ?? undefined)
+ : undefined;
+ const showAllOnTimeAxis =
+ showAllXAxisLabels && xAxisType === AxisType.Time && !!timeGrainIntervalMs;
Review Comment:
**[P1] Keep `All` working for every supported time grain.** `TIMEGRAIN_TO_TIMESTAMP` only contains hour, day, month, quarter, and year, so `P1W` and the anchored weekly grains (as well as second/minute grains) resolve to `undefined`. That leaves `showAllOnTimeAxis` false and the time axis on its old automatic tick selection. The linked issue explicitly reports Week: rendering 52 weekly points with these resulting options gives 13 month-aligned ticks, not 52 labels. Could we derive the ticks from the actual timestamps instead and add a `TimeGranularity.WEEK` regression?
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts:
##########
@@ -1273,17 +1298,19 @@ export default function transformProps(
}),
},
minorTick: { show: minorTicks },
+ // "All" wins over forceMaxInterval's own (opposite) request, since the
+ // user explicitly asked to see every label.
minInterval:
- xAxisType === AxisType.Time && resolvedTimeGrain && !forceMaxInterval
- ? (TIMEGRAIN_TO_TIMESTAMP[
- resolvedTimeGrain as keyof typeof TIMEGRAIN_TO_TIMESTAMP
- ] ?? 0)
+ xAxisType === AxisType.Time && resolvedTimeGrain
+ ? showAllOnTimeAxis || !forceMaxInterval
+ ? (timeGrainIntervalMs ?? 0)
+ : 0
: 0,
maxInterval:
- xAxisType === AxisType.Time && resolvedTimeGrain && forceMaxInterval
- ? TIMEGRAIN_TO_TIMESTAMP[
- resolvedTimeGrain as keyof typeof TIMEGRAIN_TO_TIMESTAMP
- ]
+ xAxisType === AxisType.Time && resolvedTimeGrain
+ ? showAllOnTimeAxis || forceMaxInterval
+ ? timeGrainIntervalMs
Review Comment:
**[P1] Avoid generating a tick for every empty grain bucket.** `minInterval`/`maxInterval` constrain ECharts' continuous time scale; they do not mean one tick per data point. For example, two daily points at 2010-01-01 and 2020-01-01 produce 2,898 ticks here and repeated `Exceed safe limit in TimeScale["getTicks"]` warnings, instead of two labels. Gaps are common in grouped time data, so `All` can make rendering scale with the date range and add labels where there is no point. Could we pass the actual timestamp values through ECharts 6's `axisLabel.customValues`/`axisTick.customValues` (possibly extending `resolveTemporalTickValues` from current `master`) and cover a sparse range in a test?
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(echarts): honor "All" for the x-axis label interval [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #43193:
URL: https://github.com/apache/superset/pull/43193#issuecomment-5550505417
<!-- Bito Reply -->
The flagged issue is correct. Calendar grains (like months or quarters) have variable durations, so using a fixed duration (like a constant number of milliseconds) to pin ticks can lead to misaligned labels or missing time periods when the underlying data does not perfectly align with that fixed duration.
To resolve this, you should avoid pinning the interval to a fixed duration for calendar-based grains. Instead, you can rely on ECharts' built-in time axis handling, which is designed to calculate appropriate intervals for calendar units automatically. If you need to ensure all labels are shown, you should handle the interval logic by checking if the grain is a calendar type and adjusting the `minInterval` and `maxInterval` dynamically based on the specific calendar unit, rather than a static timestamp constant.
Would you like me to fetch all other comments on this PR to validate them and implement a minimal fix for the rest?
**superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts**
```
const timeGrainIntervalMs = resolvedTimeGrain
? (TIMEGRAIN_TO_TIMESTAMP[
resolvedTimeGrain as keyof typeof TIMEGRAIN_TO_TIMESTAMP
] ?? undefined)
: undefined;
```
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(echarts): honor "All" for the x-axis label interval [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #43193:
URL: https://github.com/apache/superset/pull/43193#issuecomment-5448314081
@bikash-barnwal the category-axis part of this looks right, but I don't think it closes #36325.
ECharts only reads `axisLabel.interval` on category axes (`axisTickLabelBuilder.js` routes to `makeCategoryLabels` there); on a time axis, which is what the issue title is actually about, label placement comes from the generated ticks and `minInterval`, and never touches this option. Superset sets `xAxis.type` to `AxisType.Time` whenever the x-axis column is temporal, so the scenario people are reporting (day/week grain on a temporal column) goes through the code path this PR doesn't change.
The new regression test in `test/Timeseries/transformProps.test.ts` still uses the base fixture's `coltypes: []`, so it's exercising the category path where this already worked, not the temporal one from the report. Worth adding a case with a temporal coltype and a time grain to prove the fix actually does something there, or scoping the PR title/description to what it really covers.
Also needs a rebase, this is showing conflicts with `master`.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(echarts): honor "All" for the x-axis label interval [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #43193:
URL: https://github.com/apache/superset/pull/43193#issuecomment-5299721270
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(echarts): honor "All" for the x-axis label interval [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #43193:
URL: https://github.com/apache/superset/pull/43193#issuecomment-5764160428
Still no update since my last check-in. `requires:rebase` is still on there, and @EnxDev's points on the weekly grain and the sparse-data tick explosion are still unaddressed. Holding until those land and it's rebased on `master`.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(echarts): honor "All" for the x-axis label interval [superset]
Posted by "bikash-barnwal (via GitHub)" <gi...@apache.org>.
bikash-barnwal commented on PR #43193:
URL: https://github.com/apache/superset/pull/43193#issuecomment-5550480870
You're right, thanks for tracing that through `axisTickLabelBuilder.js` — fixed the actual gap now.
On a time axis, tick density comes from `minInterval`/`maxInterval` instead of `axisLabel.interval`. The previous commit only set `minInterval` to the resolved grain when a time grain existed, which just floors the spacing — ECharts can still choose a wider "nice" interval to fit the available width, so it wasn't actually forcing one label per point. Now when "All" is selected on a temporal axis with a resolved grain, both `minInterval` and `maxInterval` are pinned to that same grain value, which forces exactly one tick (and label) per data point. "All" wins over `forceMaxInterval`'s own (opposite) request, since the user explicitly asked to see every label. Without a resolved grain there's nothing to pin to, so that case keeps the pre-existing behavior (documented in a fallback test).
Added a regression test using a temporal coltype + Day grain (mirroring the tooltip-time-grain tests' fixture pattern) asserting `minInterval`/`maxInterval` both equal the day-in-ms value and `hideOverlap` is still forced off, plus a test for the no-resolved-grain fallback. All 4 interval-related tests pass locally. Applied the same change to MixedTimeseries.
Re: the rebase — I hit a shallow-clone issue attempting it locally (this fork's clone doesn't share full history with `apache/superset` master, so `git rebase` walked all the way back to a ~2015 root commit and conflicted there; I aborted before it did anything destructive). I don't have a safe way to fix that from here without a full unshallow fetch, so the branch is still showing conflicts against current master — flagging that honestly rather than leaving it unmentioned. Happy to redo the rebase with a proper full clone if useful, or if you'd rather just take this commit and resolve the merge conflict on your end, that works too.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(echarts): honor "All" for the x-axis label interval [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #43193:
URL: https://github.com/apache/superset/pull/43193#issuecomment-5612491903
@EnxDev's points on this seem right to me. TIMEGRAIN_TO_TIMESTAMP only covers hour/day/month/quarter/year, so weekly grains fall through and "All" still won't work for the Week case the original issue reports. The sparse-data tick explosion (thousands of ticks for a couple of far-apart points) sounds like a real problem too. Also still conflicting with `master`. Let's get those sorted before this goes further.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(echarts): honor "All" for the x-axis label interval [superset]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #43193:
URL: https://github.com/apache/superset/pull/43193#issuecomment-5298501034
## [Codecov](https://app.codecov.io/gh/apache/superset/pull/43193?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `92.30769%` with `1 line` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 66.64%. Comparing base ([`f458721`](https://app.codecov.io/gh/apache/superset/commit/f4587218dd19d046c3e4d00063e7d27f8a2ed354?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`812af79`](https://app.codecov.io/gh/apache/superset/commit/812af79c8157261165d8c776c67f1cce81e7aba1?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
| [Files with missing lines](https://app.codecov.io/gh/apache/superset/pull/43193?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [...hart-echarts/src/MixedTimeseries/transformProps.ts](https://app.codecov.io/gh/apache/superset/pull/43193?src=pr&el=tree&filepath=superset-frontend%2Fplugins%2Fplugin-chart-echarts%2Fsrc%2FMixedTimeseries%2FtransformProps.ts&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGx1Z2lucy9wbHVnaW4tY2hhcnQtZWNoYXJ0cy9zcmMvTWl4ZWRUaW1lc2VyaWVzL3RyYW5zZm9ybVByb3BzLnRz) | 66.66% | [1 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/43193?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #43193 +/- ##
=======================================
Coverage 66.64% 66.64%
=======================================
Files 2873 2873
Lines 163625 163636 +11
Branches 37767 37771 +4
=======================================
+ Hits 109053 109063 +10
- Misses 52442 52443 +1
Partials 2130 2130
```
| [Flag](https://app.codecov.io/gh/apache/superset/pull/43193/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [javascript](https://app.codecov.io/gh/apache/superset/pull/43193/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `73.76% <92.30%> (+<0.01%)` | :arrow_up: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/superset/pull/43193?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(echarts): honor "All" for the x-axis label interval [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #43193:
URL: https://github.com/apache/superset/pull/43193#issuecomment-5298374537
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(echarts): honor "All" for the x-axis label interval [superset]
Posted by "netlify[bot] (via GitHub)" <gi...@apache.org>.
netlify[bot] commented on PR #43193:
URL: https://github.com/apache/superset/pull/43193#issuecomment-5550492281
### <span aria-hidden="true">✅</span> Deploy Preview for *superset-docs-preview* ready!
| Name | Link |
|:-:|------------------------|
|<span aria-hidden="true">🔨</span> Latest commit | f0d4b6c4728df5deaa261c494e57356f53751c3a |
|<span aria-hidden="true">🔍</span> Latest deploy log | https://app.netlify.com/projects/superset-docs-preview/deploys/6a9bcd9916ab460008dcff42 |
|<span aria-hidden="true">😎</span> Deploy Preview | [https://deploy-preview-43193--superset-docs-preview.netlify.app](https://deploy-preview-43193--superset-docs-preview.netlify.app) |
|<span aria-hidden="true">📱</span> Preview on mobile | <details><summary> Toggle QR Code... </summary><br /><br /><br /><br />_Use your smartphone camera to open QR code link._</details> |
|<span aria-hidden="true">🤖</span> Make changes | [Run an agent on this branch](https://app.netlify.com/projects/superset-docs-preview/agent-runs#DcpBCoAgEADAv-w5ySIk-0HPWF0lYVMxCSP6e97mMC_kks5cYQMYwBSM9uj2oY0NW7gEo3EsQqyu3MgCmfsjlzk9O_WpUBtLWk8KzaKklCtZ75cZvh8) |
---
<!-- [superset-docs-preview Preview](https://deploy-preview-43193--superset-docs-preview.netlify.app) -->
_To edit notification comments on pull requests, go to your [Netlify project configuration](https://app.netlify.com/projects/superset-docs-preview/configuration/notifications#deploy-notifications)._
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(echarts): honor "All" for the x-axis label interval [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #43193:
URL: https://github.com/apache/superset/pull/43193#issuecomment-5551355820
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers an incremental AI Review.
- `/review full` - Manually triggers a full AI Review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(echarts): honor "All" for the x-axis label interval [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #43193:
URL: https://github.com/apache/superset/pull/43193#discussion_r3939965287
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts:
##########
@@ -1204,6 +1205,26 @@ export default function transformProps(
xAxisType === AxisType.Time &&
xAxisLabelRotation === 0 &&
!!resolvedTimeGrain;
+ const {
+ interval: xAxisLabelIntervalValue,
+ showAllLabels: showAllXAxisLabels,
+ } = getAxisLabelInterval(xAxisLabelInterval);
+ // axisLabel.interval is only ever consulted for category axes in ECharts
+ // (axisTickLabelBuilder routes to makeCategoryLabels there; a time axis
+ // goes through makeRealNumberLabels and never reads it), and Superset sets
+ // xAxis.type to Time whenever the x-axis column is temporal -- the common
+ // case #36325 actually reports. On a time axis, tick density is governed
+ // by minInterval/maxInterval instead, and minInterval alone only floors
+ // the spacing: ECharts can still choose a wider "nice" interval to fit the
+ // available width. Pinning both bounds to the resolved grain forces one
+ // tick, and therefore one label, per data point.
+ const timeGrainIntervalMs = resolvedTimeGrain
+ ? (TIMEGRAIN_TO_TIMESTAMP[
+ resolvedTimeGrain as keyof typeof TIMEGRAIN_TO_TIMESTAMP
+ ] ?? undefined)
+ : undefined;
Review Comment:
**Suggestion:** Calendar grains use fixed durations, so pinning a month, quarter, or year can create ticks on drifting dates and labels for missing time periods. [logic error]
**Assessment:** 🟠 `Major` · 🔁 `Occurrence: Sometimes`
[](https://docs.codeant.ai/cli/resolve-pr-comments-skill) [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=f4d973040c224f5085f99c23080bd2f6&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=f4d973040c224f5085f99c23080bd2f6&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
<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:** 1221:1225
**Comment:**
*Logic Error: Calendar grains use fixed durations, so pinning a month, quarter, or year can create ticks on drifting dates and labels for missing time periods.
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%2F43193&comment_hash=34fe75cea44be601378d535594190511bf80f53f51e8c4e28c5360ff1d561b1f&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F43193&comment_hash=34fe75cea44be601378d535594190511bf80f53f51e8c4e28c5360ff1d561b1f&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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(echarts): honor "All" for the x-axis label interval [superset]
Posted by "EnxDev (via GitHub)" <gi...@apache.org>.
EnxDev commented on PR #43193:
URL: https://github.com/apache/superset/pull/43193#issuecomment-5339664912
## EnxDev's Review Agent — apache/superset#43193 · HEAD 8e9c772
**comment** — the category-axis half is correct and well-argued, but on a temporal x-axis (the case in #36325) ECharts still ignores `axisLabel.interval`, so "All" remains unhonored there.
### 🔴 Functional
- **`superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts:1271`** (and `MixedTimeseries/transformProps.ts:782`) · _High_ — ECharts only reads `axisLabel.interval` on **category** axes. In echarts 6.1.0, `coord/axisTickLabelBuilder.js:81`:
```js
// Only ordinal scale support tick interval
return axis.type === 'category' ? makeCategoryLabels(axis, ctx) : makeRealNumberLabels(axis);
```
and `coord/axisHelper.js:199` gates the same way:
```js
export function shouldShowAllLabels(axis) {
return axis.type === 'category' && getOptionCategoryInterval(axis.getLabelModel()) === 0;
}
```
Issue #36325 is an Area chart over a temporal x-axis with a day/week grain, so `getAxisType` (`src/utils/series.ts:938`) returns `AxisType.Time` and the newly-coerced `interval: 0` is dropped just as the string `'0'` was. On a time axis the label set is `axis.scale.getTicks()` mapped 1:1 (`makeRealNumberLabels`), bounded by the `minInterval`/`maxInterval` this file already derives from the time grain. The only change that lands there is `hideOverlap: false`, which un-hides *colliding* labels but cannot add ticks the scale never generated — and the reported "every 4th label" thinning comes from the nice-interval tick algorithm, not from collision hiding. Net: the linked issue's scenario is unchanged.
Fix: when `showAllXAxisLabels && xAxisType === AxisType.Time`, also set the **axis-level** `interval` (value units, i.e. ms — not `axisLabel.interval`) to `TIMEGRAIN_TO_TIMESTAMP[resolvedTimeGrain]`. ECharts forwards that to the scale as `userInterval` (`coord/axisNiceTicks.js:183`), which forces one tick per grain; the map is already imported here for `minInterval`. **regression test:** `transformProps` with `coltypes: [GenericDataType.Temporal]`, a `time_grain_sqla`, and `xAxisLabelInterval: '0'`, asserting the forced tick spacing on `xAxis.interval` — not just `axisLabel.interval === 0`, which passes today regardless.
### 🟡 Should-fix
- **`superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/transformProps.test.ts:2441`** — the fixture leaves `coltypes: []`, so `xAxisDataType` is `undefined` and `getAxisType` falls through to `AxisType.Category` (`src/utils/series.ts:948`). The new test therefore covers only the axis type where `interval` already works, and asserts the option object rather than label output — both assertions stay green even with the temporal path broken. Add a temporal-coltype case.
- **`superset-frontend/plugins/plugin-chart-echarts/src/MixedTimeseries/transformProps.ts:777`** — same behavioral change, no test; codecov's single uncovered patch line is this branch. Add a MixedTimeseries "All" case mirroring the Timeseries one.
### 🔵 Nits
- `superset-frontend/plugins/plugin-chart-echarts/src/utils/series.ts:1137` — `Number('')` is `0`, so a stored or imported `xAxisLabelInterval: ''` silently becomes "show every label, `hideOverlap` off". The control can't emit it (`freeForm: false, clearable: false` in `src/controls.tsx:371`), but form_data arriving via the API or an import can. Guard the empty string; the helper test covers `null`/`undefined`/`'nope'` but not `''`.
### 🙌 Praise
- `superset-frontend/plugins/plugin-chart-echarts/src/utils/series.ts:1129` — the `hideOverlap` half is right and non-obvious: `AxisBuilder.js:604` applies `hideOverlap` independently of the category interval, so `interval: 0` alone would still get overridden. Deferring Bubble is also correctly reasoned — it sets the axis-level `interval`, a different option in value units.
<!-- enxdev-review-agent:8e9c772 -->
_Reviewed by EnxDev's Review Agent — @EnxDev · HEAD 8e9c772._
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org