codeant-ai-for-open-source[bot] commented on code in PR #43669:
URL: https://github.com/apache/superset/pull/43669#discussion_r3885618812
##########
superset-frontend/plugins/plugin-chart-echarts/src/utils/formatters.ts:
##########
@@ -213,3 +215,105 @@ export function getXAxisFormatter(
}
return String;
}
+
+type XAxisFormatterFn =
+ | TimeFormatter
+ | NumberFormatter
+ | StringConstructor
+ | ((value: number | string) => string);
+
+/**
+ * Wraps an x-axis time formatter so that:
+ * - consecutive ticks that format to identical text are blanked (e.g. the
+ * boundary label forced by showMaxLabel duplicating the last real tick).
+ * - ticks that would render close enough to visually collide with the
+ * previously shown label are blanked, since disabling ECharts'
+ * `hideOverlap` (required to keep the forced boundary label visible, see
+ * #39899) also disables its native overlap suppression for every other
+ * label on the axis.
+ *
+ * The forced axis boundary labels (domainMin/domainMax) are never blanked by
+ * the spacing check so they stay visible regardless of density.
+ */
+export function createSpacedXAxisFormatter(
+ xAxisFormatter: XAxisFormatterFn | undefined,
+ domainMin: number | undefined,
+ domainMax: number | undefined,
+ plotWidthPx: number,
+): (value: number | string) => string {
+ const pixelsPerMs =
+ domainMin !== undefined && domainMax !== undefined && domainMax > domainMin
+ ? plotWidthPx / (domainMax - domainMin)
+ : undefined;
+ let lastLabel: string | undefined;
+ let lastValue: number | undefined;
+ let lastShownValue: number | undefined;
+ const wrapper = (value: number | string) => {
+ // ECharts formats the labels in repeated ascending passes. Reset the
+ // dedup/spacing state when the sequence restarts so a forced boundary
+ // label (e.g. the min date) isn't blanked by the previous pass's state
+ // when both format identically (e.g. a May-to-May range).
+ if (
+ typeof value === 'number' &&
+ lastValue !== undefined &&
+ value <= lastValue
+ ) {
+ lastLabel = undefined;
+ lastShownValue = undefined;
+ }
+ if (typeof value === 'number') {
+ lastValue = value;
+ }
+ const label =
+ typeof xAxisFormatter === 'function'
+ ? (xAxisFormatter as Function)(value)
+ : String(value);
+ if (label === lastLabel) {
+ return '';
+ }
+ const isBoundary =
+ typeof value === 'number' && (value === domainMin || value ===
domainMax);
+ if (
+ !isBoundary &&
+ typeof value === 'number' &&
+ pixelsPerMs !== undefined &&
+ lastShownValue !== undefined &&
+ (value - lastShownValue) * pixelsPerMs <
+ label.length * TIMESERIES_CONSTANTS.xAxisLabelCharWidthPx +
+ TIMESERIES_CONSTANTS.xAxisLabelMinGapPx
+ ) {
+ return '';
+ }
+ lastLabel = label;
+ if (typeof value === 'number') {
+ lastShownValue = value;
+ }
+ return label;
+ };
+ if (typeof xAxisFormatter === 'function' && 'id' in xAxisFormatter) {
+ (wrapper as { id?: unknown }).id = (xAxisFormatter as { id?: unknown }).id;
+ }
+ return wrapper;
+}
+
+/**
+ * Computes the [min, max] of a temporal x-axis column across one or more
+ * data record arrays, for use with createSpacedXAxisFormatter.
+ */
+export function getXAxisDomain(
+ dataRecordArrays: Record<string, unknown>[][],
+ xAxisCol: string,
+): [number | undefined, number | undefined] {
+ let domainMin: number | undefined;
+ let domainMax: number | undefined;
+ dataRecordArrays.forEach(records => {
+ records.forEach(record => {
+ const value = record[xAxisCol];
+ if (typeof value === 'number') {
+ if (domainMin === undefined || value < domainMin) domainMin = value;
+ if (domainMax === undefined || value > domainMax) domainMax = value;
+ }
+ });
Review Comment:
**Suggestion:** `getXAxisDomain` only recognizes numeric values, but
temporal query rows can contain timestamp strings or `Date` objects. For those
charts both bounds remain undefined, so `pixelsPerMs` is disabled and, because
`hideOverlap` is simultaneously set to false, no overlap suppression is applied
at all. Normalize temporal values to epoch milliseconds before calculating the
domain. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Dense string-timestamp charts retain overlapping labels.
- ❌ Date-valued query records bypass spacing suppression.
- ⚠️ Timeseries and MixedTimeseries are both affected.
```
</details>
[](https://docs.codeant.ai/cli/resolve-pr-comments-skill)
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=d13d195cc8e546298930f86d060637ca&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=d13d195cc8e546298930f86d060637ca&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/utils/formatters.ts
**Line:** 311:316
**Comment:**
*Logic Error: `getXAxisDomain` only recognizes numeric values, but
temporal query rows can contain timestamp strings or `Date` objects. For those
charts both bounds remain undefined, so `pixelsPerMs` is disabled and, because
`hideOverlap` is simultaneously set to false, no overlap suppression is applied
at all. Normalize temporal values to epoch milliseconds before calculating the
domain.
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%2F43669&comment_hash=49dd02242cae9d183670a8e9d28c2f95a3fb18eb70ce3b46bd2c6215385dfbd8&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F43669&comment_hash=49dd02242cae9d183670a8e9d28c2f95a3fb18eb70ce3b46bd2c6215385dfbd8&reaction=dislike'>👎</a>
##########
superset-frontend/plugins/plugin-chart-echarts/src/constants.ts:
##########
@@ -52,6 +52,12 @@ export const TIMESERIES_CONSTANTS = {
microChartHeight: 60,
// One y-axis tick per this many pixels of chart height
yAxisPixelsPerTick: 80,
+ // Rough average glyph width (px) used to estimate whether adjacent x-axis
+ // time labels would visually collide, since the real rendered width isn't
+ // known until ECharts lays out the axis.
+ xAxisLabelCharWidthPx: 7,
+ // Minimum gap (px) to keep between adjacent x-axis time labels.
+ xAxisLabelMinGapPx: 8,
Review Comment:
**Suggestion:** The collision threshold assumes every rendered character is
exactly seven pixels wide, but the formatter is also used for localized and
custom time formats. Wide glyphs or long localized month names can exceed this
estimate, causing the wrapper to retain labels whose actual rendered footprints
overlap; narrow glyphs can also cause valid labels to be unnecessarily removed.
Use measured text width or a conservative width calculation based on the actual
formatter output and font. [possible bug]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Custom time formats can remain visually overlapped.
- ⚠️ Localized month labels may be thinned inaccurately.
- ⚠️ Non-overlapping labels may be unnecessarily hidden.
```
</details>
[](https://docs.codeant.ai/cli/resolve-pr-comments-skill)
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=b1f7557945fe4d5e97293cbcb50617b4&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=b1f7557945fe4d5e97293cbcb50617b4&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/constants.ts
**Line:** 58:60
**Comment:**
*Possible Bug: The collision threshold assumes every rendered character
is exactly seven pixels wide, but the formatter is also used for localized and
custom time formats. Wide glyphs or long localized month names can exceed this
estimate, causing the wrapper to retain labels whose actual rendered footprints
overlap; narrow glyphs can also cause valid labels to be unnecessarily removed.
Use measured text width or a conservative width calculation based on the actual
formatter output and font.
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%2F43669&comment_hash=e71b5c5c8a282cc474253a7492fe78dbe9772516c0a710905c695b03b6cbe51b&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F43669&comment_hash=e71b5c5c8a282cc474253a7492fe78dbe9772516c0a710905c695b03b6cbe51b&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]
