Skip to content

feat(echarts): make bar chart label position user-configurable#38695

Open
sancho11 wants to merge 2 commits intoapache:masterfrom
sancho11:fix/echarts-bar-label-overflow
Open

feat(echarts): make bar chart label position user-configurable#38695
sancho11 wants to merge 2 commits intoapache:masterfrom
sancho11:fix/echarts-bar-label-overflow

Conversation

@sancho11
Copy link
Copy Markdown

@sancho11 sancho11 commented Mar 17, 2026

User description

SUMMARY

Bar charts with "Show Values" enabled had their data label positions hardcoded based on orientation: vertical bars always placed labels at top, and horizontal bars always placed them inside. This caused a confusing UX on stacked vertical bar charts where labels would appear on top of the wrong segment (the label for one segment would visually sit inside the adjacent segment above it).

This PR adds a Label Position dropdown to the Customize panel (visible when "Show Values" is enabled) with the following options: Auto, Top, Inside, Bottom, Left, Right.

Auto (default) preserves the existing orientation-aware behavior: vertical bars use top, horizontal bars use inside (as requested at issue #27553) , so there is no breaking change for existing charts.

Users can now explicitly select Inside to fix label placement on stacked vertical bars, or any other position as needed.
The control is also added to Mixed Timeseries charts independently for Query A and Query B.

BEFORE/AFTER

Horizontal Example:

Before:
before

After:
after

Vertical Example: (User input inside)

newselector

Before:
beforev

After:
afterv

TESTING INSTRUCTIONS

  1. Create a Bar Chart (vertical orientation)
  2. In the Customize tab, enable Show Values
  3. Verify a Label Position dropdown appears with options: Auto, Top, Inside, Bottom, Left, Right
  4. Select Inside, labels should move inside each bar segment
  5. Select Top, labels should appear above each bar
  6. Enable Stacked Style → Stack and disable Only Total, with Inside selected, labels should appear within each segment (not on top of the wrong segment)
  7. Switch to Horizontal orientation, Auto should default to inside, manual selections should still work
  8. Create a Mixed Timeseries chart, enable Show Values for Query A and/or Query B, each should have its own independent Label Position control

ADDITIONAL INFORMATION


CodeAnt-AI Description

Make bar label position configurable and prevent label overflow

What Changed

  • Adds a "Label Position" control (Auto, Top, Inside, Bottom, Left, Right) to bar chart and mixed timeseries Customize panels when "Show Values" is enabled; default "Auto" preserves orientation-aware behavior.
  • Chart rendering now uses the chosen label position (or falls back to orientation-aware defaults) so labels on stacked vertical bars can be placed inside the correct segment.
  • Long/overflowing data labels are truncated to avoid overlapping or layout shifts; negative bar values are still positioned outside.
  • Mixed timeseries supports independent label position settings for Query A and Query B; unit tests added to verify label position and overflow behavior.

Impact

✅ Clearer stacked bar labels
✅ Fewer misplaced labels on stacked vertical bars
✅ Truncated overflowing labels to avoid layout shifts

💡 Usage Guide

Checking Your Pull Request

Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.

Talking to CodeAnt AI

Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:

@codeant-ai ask: Your question here

This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.

Example

@codeant-ai ask: Can you suggest a safer alternative to storing this secret?

Preserve Org Learnings with CodeAnt

You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:

@codeant-ai: Your feedback here

This helps CodeAnt AI learn and adapt to your team's coding style and standards.

Example

@codeant-ai: Do not flag unused imports.

Retrigger review

Ask CodeAnt AI to review the PR again, by typing:

@codeant-ai: review

Check Your Repository Health

To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.

Adds a 'Label Position' dropdown to the bar chart Customize panel,
allowing users to choose where data labels appear (Auto, Top, Inside,
Bottom, Left, Right).

Previously the position was hardcoded by orientation: vertical bars
always used 'top' and horizontal bars always used 'inside'. This caused
labels to overlap the wrong segment on stacked vertical bar charts.

The new 'Auto' default preserves the existing orientation-aware
behavior. Users can now explicitly select 'Inside' to fix label
placement on stacked bars, or any other position as needed.

The control is also added to Mixed Timeseries charts independently for
Query A and Query B.

Fixes apache#27553
Closes apache#33135
@bito-code-review
Copy link
Copy Markdown
Contributor

bito-code-review bot commented Mar 17, 2026

Code Review Agent Run #0b8ff3

Actionable Suggestions - 0
Additional Suggestions - 2
  • superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts - 1
    • Padding calculation may be insufficient · Line 783-789
      The padding.right calculation for rotated x-axis labels now excludes TIMESERIES_CONSTANTS.gridOffsetRight (20px), potentially reducing padding and causing label clipping. The original code added this base offset before taking max with the rotation-based padding.
  • superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/transformProps.test.ts - 1
    • TypeScript `any` usage violation · Line 689-689
      The new test cases use `as any[]` for the transformed series, which violates the project's TypeScript standards that prohibit `any` types. Since `SeriesOption` from echarts is already imported, use that for type safety instead.
      Code suggestion
       @@ -688,2 +688,2 @@
      -    const transformedSeries = transformProps(chartProps).echartOptions
      -      .series as any[];
      +    const transformedSeries = transformProps(chartProps).echartOptions
      +      .series as SeriesOption[];
Review Details
  • Files reviewed - 9 · Commit Range: eae83bb..eae83bb
    • superset-frontend/plugins/plugin-chart-echarts/src/MixedTimeseries/controlPanel.tsx
    • superset-frontend/plugins/plugin-chart-echarts/src/MixedTimeseries/transformProps.ts
    • superset-frontend/plugins/plugin-chart-echarts/src/MixedTimeseries/types.ts
    • superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/constants.ts
    • superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts
    • superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformers.ts
    • superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/types.ts
    • superset-frontend/plugins/plugin-chart-echarts/src/controls.tsx
    • superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/transformProps.test.ts
  • Files skipped - 0
  • Tools
    • Whispers (Secret Scanner) - ✔︎ Successful
    • Detect-secrets (Secret Scanner) - ✔︎ Successful
    • Eslint (Linter) - ✔︎ Successful

Bito Usage Guide

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 documentation for additional commands.

Configuration

This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.

Documentation & Help

AI Code Review powered by Bito Logo

@codeant-ai-for-open-source codeant-ai-for-open-source bot added the size:L This PR changes 100-499 lines, ignoring generated files label Mar 17, 2026
@codeant-ai-for-open-source
Copy link
Copy Markdown
Contributor

Sequence Diagram

This PR adds a user-facing Label Position control for bar value labels and wires it through chart transformation logic. The flow keeps Auto as the default orientation-aware behavior while allowing explicit positions, including separate settings for mixed timeseries query groups.

sequenceDiagram
    participant User
    participant CustomizePanel
    participant TransformProps
    participant SeriesTransformer
    participant Chart

    User->>CustomizePanel: Enable show values and choose label position
    CustomizePanel->>TransformProps: Submit form data with label position
    TransformProps->>SeriesTransformer: Pass label position for each bar series
    SeriesTransformer->>SeriesTransformer: Use auto by orientation or user selected position
    SeriesTransformer-->>TransformProps: Return label config with position and truncate overflow
    TransformProps-->>Chart: Render bars with configured label placement
Loading

Generated by CodeAnt AI

Comment on lines +474 to +475
max: yAxisMax,
formatter: seriesFormatter,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: The max bound used for hiding bar labels is tied to Query A instead of the axis the series is actually rendered on. If Query A is assigned to the secondary axis, labels will be hidden using the primary-axis max, which produces incorrect label visibility. Use the series axis index to pick the correct max bound. [logic error]

Severity Level: Major ⚠️
- ⚠️ Mixed Chart bar labels hide using wrong axis bound.
- ⚠️ Query A on secondary axis shows inconsistent value labels.
Suggested change
max: yAxisMax,
formatter: seriesFormatter,
max: yAxisIndex === 1 ? maxSecondary : yAxisMax,
formatter: seriesFormatter,
Steps of Reproduction ✅
1. In Explore, select **Mixed Chart** (plugin wiring at `MixedTimeseries/index.ts:50-86`
uses `transformProps`), set Query A `seriesType` to Bar and enable `show_value` (controls
defined in `MixedTimeseries/controlPanel.tsx:145-202`).

2. In Query A customize, set `yAxisIndex` to Secondary via `yAxisIndex` control
(`MixedTimeseries/controlPanel.tsx:293-306`), and set different primary/secondary max
bounds via `y_axis_bounds` and `y_axis_bounds_secondary` (`controlPanel.tsx:177-231`).

3. Render chart: Query A series goes through `rawSeriesA.forEach` in
`MixedTimeseries/transformProps.ts:416-489`; for bar series, formatter is wrapped by
`getOverMaxHiddenFormatter` with `max: yAxisMax` at `transformProps.ts:473-476`, even when
`yAxisIndex` is `1`.

4. `getOverMaxHiddenFormatter` hides labels when `value > max` (`utils/series.ts:41-48`),
so Query A labels are filtered against the **primary** max instead of secondary max,
producing incorrect label visibility.
Prompt for AI Agent 🤖
This is a comment left during a code review.

**Path:** superset-frontend/plugins/plugin-chart-echarts/src/MixedTimeseries/transformProps.ts
**Line:** 474:475
**Comment:**
	*Logic Error: The max bound used for hiding bar labels is tied to Query A instead of the axis the series is actually rendered on. If Query A is assigned to the secondary axis, labels will be hidden using the primary-axis max, which produces incorrect label visibility. Use the series axis index to pick the correct max bound.

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.
👍 | 👎

Comment on lines +549 to +550
max: maxSecondary,
formatter: seriesFormatter,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: Query B also hardcodes the max bound to the secondary-axis max, even when the series is configured to render on the primary axis. This causes incorrect hiding/showing of value labels whenever Query B uses yAxisIndexB = 0. Select the max bound based on the actual axis index. [logic error]

Severity Level: Major ⚠️
- ⚠️ Query B labels ignore primary-axis max when axis switched.
- ⚠️ Mixed Chart value labels appear/disappear unpredictably.
Suggested change
max: maxSecondary,
formatter: seriesFormatter,
max: yAxisIndexB === 1 ? maxSecondary : yAxisMax,
formatter: seriesFormatter,
Steps of Reproduction ✅
1. Open **Mixed Chart** (registered with `transformProps` in
`MixedTimeseries/index.ts:50-86`), configure Query B as Bar with `show_valueB` enabled
(`controlPanel.tsx:120-122`, and generated controls in `createCustomizeSection`).

2. Set Query B `yAxisIndexB` to Primary (value `0`) using the Y Axis selector
(`MixedTimeseries/controlPanel.tsx:293-306`, suffix `B` path), and set distinct max values
for primary and secondary bounds (`controlPanel.tsx:177-231`).

3. During render, Query B loop (`MixedTimeseries/transformProps.ts:492-565`) applies
`getOverMaxHiddenFormatter` with hardcoded `max: maxSecondary` at
`transformProps.ts:548-551`, regardless of `yAxisIndexB`.

4. Formatter logic in `utils/series.ts:41-48` hides labels using that wrong max, so Query
B on primary axis incorrectly hides/shows labels based on secondary-axis bound.
Prompt for AI Agent 🤖
This is a comment left during a code review.

**Path:** superset-frontend/plugins/plugin-chart-echarts/src/MixedTimeseries/transformProps.ts
**Line:** 549:550
**Comment:**
	*Logic Error: Query B also hardcodes the max bound to the secondary-axis max, even when the series is configured to render on the primary axis. This causes incorrect hiding/showing of value labels whenever Query B uses `yAxisIndexB = 0`. Select the max bound based on the actual axis index.

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.
👍 | 👎

Comment on lines 159 to 166
return axisValue < 0
? {
value,
label: {
position: 'outside',
},
}
value,
label: {
position: 'outside',
},
}
: value;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: Negative bar values always force a per-point label position of outside, which overrides the new user-configured label position and breaks the expected behavior of the Label Position control for charts containing negative values. Use the series label position when available and only fall back to outside as a default. [logic error]

Severity Level: Major ⚠️
- ⚠️ Label Position control inconsistent for negative bar values.
- ⚠️ Timeseries and MixedTimeseries bar label UX diverges.
Suggested change
return axisValue < 0
? {
value,
label: {
position: 'outside',
},
}
value,
label: {
position: 'outside',
},
}
: value;
return axisValue < 0
? {
value,
label: {
position:
(series as any)?.label?.position && (series as any)?.label?.position !== 'auto'
? (series as any).label.position
: 'outside',
},
}
: value;
Steps of Reproduction ✅
1. In Explore, build a Timeseries Bar chart with `show_value=true`, `stack=null`,
`label_position='top'`, and data containing negatives (normal UI path wired through
`src/Timeseries/transformProps.ts:259-297`).

2. Rendering calls `transformSeries()` from `src/Timeseries/transformProps.ts:259` (also
`src/MixedTimeseries/transformProps.ts:299` and `:373` for mixed charts), passing the
user-selected `labelPosition`.

3. In `src/Timeseries/transformers.ts:241-243`, bar series without stack always run
`transformNegativeLabelsPosition(series, isHorizontal)`.

4. `transformNegativeLabelsPosition()` in `src/Timeseries/transformers.ts:159-166` assigns
per-point `label.position='outside'` for negatives, which overrides series-level
`label.position` set later in `transformSeries`
(`src/Timeseries/transformers.ts:274-282`), so user-selected position is ignored on
negative bars.
Prompt for AI Agent 🤖
This is a comment left during a code review.

**Path:** superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformers.ts
**Line:** 159:166
**Comment:**
	*Logic Error: Negative bar values always force a per-point label position of `outside`, which overrides the new user-configured label position and breaks the expected behavior of the Label Position control for charts containing negative values. Use the series label position when available and only fall back to `outside` as a default.

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.
👍 | 👎

Comment on lines 758 to 761
yAxisTitlePosition === 'Left'
? TIMESERIES_CONSTANTS.gridOffsetLeft +
(Number(yAxisTitleMargin) || 0)
(Number(yAxisTitleMargin) || 0)
: TIMESERIES_CONSTANTS.gridOffsetLeft,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: Left grid padding now always adds yAxisTitleMargin when yAxisTitlePosition is Left, even when no Y-axis title is shown, causing unnecessary chart offset and reduced plotting area. Gate the extra margin by addYAxisTitleOffset so margin is only applied when a Y-axis title is actually present. [logic error]

Severity Level: Major ⚠️
- ⚠️ Empty-title charts can get unnecessary left plot offset.
- ⚠️ Reduced plotting width in Timeseries and MixedTimeseries.
Suggested change
yAxisTitlePosition === 'Left'
? TIMESERIES_CONSTANTS.gridOffsetLeft +
(Number(yAxisTitleMargin) || 0)
(Number(yAxisTitleMargin) || 0)
: TIMESERIES_CONSTANTS.gridOffsetLeft,
left:
yAxisTitlePosition === 'Left'
? TIMESERIES_CONSTANTS.gridOffsetLeft +
(addYAxisTitleOffset ? Number(yAxisTitleMargin) || 0 : 0)
: TIMESERIES_CONSTANTS.gridOffsetLeft,
Steps of Reproduction ✅
1. Configure a chart with empty Y-axis title (`yAxisTitle=''`),
`y_axis_title_position='Left'`, and default `y_axis_title_margin=15` (defaults from
`src/constants.ts:15-20` / `src/Timeseries/constants.ts:12-16`; controls exposed
independently in `src/Timeseries/Regular/Bar/controlPanel.tsx:53-66`).

2. In `src/Timeseries/transformProps.ts:33-46`, `addYAxisLabelOffset` becomes false when
title is empty, but `getPadding()` still receives `yAxisTitlePosition` and
`yAxisTitleMargin`.

3. In `src/Timeseries/transformers.ts:758-761`, left padding adds
`(Number(yAxisTitleMargin) || 0)` whenever position is `'Left'`, regardless of
`addYAxisTitleOffset`.

4. Result: grid left padding increases even with no title, shrinking/offsetting plot area;
same call pattern also exists in mixed charts at
`src/MixedTimeseries/transformProps.ts:436-451`.
Prompt for AI Agent 🤖
This is a comment left during a code review.

**Path:** superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformers.ts
**Line:** 758:761
**Comment:**
	*Logic Error: Left grid padding now always adds `yAxisTitleMargin` when `yAxisTitlePosition` is `Left`, even when no Y-axis title is shown, causing unnecessary chart offset and reduced plotting area. Gate the extra margin by `addYAxisTitleOffset` so margin is only applied when a Y-axis title is actually present.

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.
👍 | 👎

@netlify
Copy link
Copy Markdown

netlify bot commented Mar 17, 2026

Deploy Preview for superset-docs-preview ready!

Name Link
🔨 Latest commit eae83bb
🔍 Latest deploy log https://app.netlify.com/projects/superset-docs-preview/deploys/69b9899f44e76d000857d4f6
😎 Deploy Preview https://deploy-preview-38695--superset-docs-preview.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

@sadpandajoe
Copy link
Copy Markdown
Member

@yousoph @kasiazjc I think we recently did something like this and rolled it back. Pinging in case if either of you have opinions on this feature.

@sadpandajoe sadpandajoe changed the title fix(echarts): make bar chart label position user-configurable feat(echarts): make bar chart label position user-configurable Mar 17, 2026
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR makes ECharts Timeseries Bar (and Mixed Timeseries) value-label placement user-configurable by introducing a “Label Position” control and wiring it through the Timeseries/MixedTimeseries transformers.

Changes:

  • Adds a new “Label Position” select control (default: auto) to Timeseries Bar controls and Mixed Timeseries (Query A/B) controls.
  • Threads labelPosition / labelPositionB through transformProps into transformSeries, and applies label position plus overflow: 'truncate'.
  • Adds unit tests for Timeseries labelPosition behavior (auto/manual) and overflow.

Reviewed changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 8 comments.

Show a summary per file
File Description
superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/transformProps.test.ts Adds unit tests asserting label position + overflow behavior.
superset-frontend/plugins/plugin-chart-echarts/src/controls.tsx Introduces shared labelPositionControl and includes it in show-value sections.
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/types.ts Extends Timeseries form data/types to include labelPosition.
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformers.ts Applies configurable label position logic and truncation overflow in transformSeries.
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts Plumbs labelPosition from formData into transformSeries.
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/constants.ts Adds labelPosition: 'auto' to default Timeseries form data.
superset-frontend/plugins/plugin-chart-echarts/src/MixedTimeseries/types.ts Adds labelPosition/labelPositionB to Mixed Timeseries form data and defaults.
superset-frontend/plugins/plugin-chart-echarts/src/MixedTimeseries/transformProps.ts Plumbs labelPosition/labelPositionB into transformSeries for Query A/B.
superset-frontend/plugins/plugin-chart-echarts/src/MixedTimeseries/controlPanel.tsx Adds “Label Position” control per query in the Mixed Timeseries customize section.
Comments suppressed due to low confidence (1)

superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts:650

  • Default hook no-ops are written as () => { } which is inconsistent with the surrounding codebase’s formatting and will likely fail prettier/eslint checks. Use the standard empty block form (() => {}) to avoid CI/pre-commit formatting failures.
  const {
    setDataMask = () => { },
    setControlValue = () => { },
    onContextMenu,
    onLegendStateChanged,
    onLegendScroll,
  } = hooks;

You can also share your feedback on Copilot code review. Take the survey.

Comment on lines 180 to 185
showLegend,
showValue,
showValueB,
labelPosition,
labelPositionB,
onlyTotal,
Comment on lines 758 to 761
yAxisTitlePosition === 'Left'
? TIMESERIES_CONSTANTS.gridOffsetLeft +
(Number(yAxisTitleMargin) || 0)
(Number(yAxisTitleMargin) || 0)
: TIMESERIES_CONSTANTS.gridOffsetLeft,
Comment on lines +688 to +690
const transformedSeries = transformProps(chartProps).echartOptions
.series as any[];

],
default: 'auto',
renderTrigger: true,
description: t('Position of the data label relative to the bar segment'),
Comment on lines +161 to +162
Boolean(controls?.show_value?.value) ||
Boolean(controls?.show_valueB?.value),
Comment on lines +413 to +421
label: {
show: !!showValue,
position: isHorizontal ? 'right' : 'top',
position: (
labelPosition === 'auto' || !labelPosition
? isHorizontal
? 'inside'
: 'top'
: labelPosition
) as 'top' | 'bottom' | 'left' | 'right' | 'inside' | 'insideTop' | 'insideBottom' | 'insideLeft' | 'insideRight',
);

const { setDataMask = () => {}, onContextMenu } = hooks;
const { setDataMask = () => { }, onContextMenu } = hooks;
Comment on lines +206 to +226
name: `label_position${controlSuffix}`,
config: {
type: 'SelectControl',
freeForm: false,
label: t('Label Position'),
choices: [
['auto', t('Auto')],
['top', t('Top')],
['inside', t('Inside')],
['bottom', t('Bottom')],
['left', t('Left')],
['right', t('Right')],
],
default: 'auto',
renderTrigger: true,
description: t(
'Position of the data label relative to the bar segment',
),
visibility: ({ controls }: ControlPanelsContainerProps) =>
Boolean(controls?.[`show_value${controlSuffix}`]?.value),
},
@codeant-ai-for-open-source codeant-ai-for-open-source bot added size:L This PR changes 100-499 lines, ignoring generated files and removed size:L This PR changes 100-499 lines, ignoring generated files labels Mar 19, 2026
Comment on lines +212 to +219
['top', t('Top')],
['inside', t('Inside')],
['bottom', t('Bottom')],
['left', t('Left')],
['right', t('Right')],
],
default: 'top',
renderTrigger: true,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: The new label-position control omits the auto option and hardcodes the default to top, which breaks the expected contract used elsewhere (labelPosition defaults to auto) and prevents users from restoring orientation-aware behavior. Add auto to the choices and make it the default so mixed timeseries stays consistent with existing chart defaults and transform logic. [logic error]

Severity Level: Major ⚠️
- ⚠️ Mixed chart UI deviates from shared label contract.
- ⚠️ Users cannot reselect auto label behavior.
- ⚠️ Form-data defaults inconsistent across ECharts timeseries charts.
Suggested change
['top', t('Top')],
['inside', t('Inside')],
['bottom', t('Bottom')],
['left', t('Left')],
['right', t('Right')],
],
default: 'top',
renderTrigger: true,
['auto', t('Auto')],
['top', t('Top')],
['inside', t('Inside')],
['bottom', t('Bottom')],
['left', t('Left')],
['right', t('Right')],
],
default: 'auto',
Steps of Reproduction ✅
1. Open Mixed Chart Explore UI (plugin wired via `MixedTimeseries/index.ts:49-52` using
`controlPanel` and `transformProps`).

2. In Customize section (`MixedTimeseries/controlPanel.tsx:206-229`), enable `show_value`
then inspect `label_position`: options are only top/inside/bottom/left/right and default
is hardcoded `'top'`.

3. Compare shared ECharts label control (`controls.tsx:143-29`) and timeseries defaults
(`Timeseries/constants.ts:89`): both include `'auto'` and default to `'auto'`.

4. Render path uses `MixedTimeseries/transformProps.ts:228` (merges defaults/formData)
then passes `labelPosition` to `transformSeries` (`transformProps.ts:483,558`), where
`'auto'` has explicit fallback behavior (`Timeseries/transformers.ts:416-421`). With
current control, users cannot select/restore `'auto'` from UI, creating an inconsistent
contract.
Prompt for AI Agent 🤖
This is a comment left during a code review.

**Path:** superset-frontend/plugins/plugin-chart-echarts/src/MixedTimeseries/controlPanel.tsx
**Line:** 212:219
**Comment:**
	*Logic Error: The new label-position control omits the `auto` option and hardcodes the default to `top`, which breaks the expected contract used elsewhere (`labelPosition` defaults to `auto`) and prevents users from restoring orientation-aware behavior. Add `auto` to the choices and make it the default so mixed timeseries stays consistent with existing chart defaults and transform 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.
👍 | 👎

@sancho11
Copy link
Copy Markdown
Author

Hello! Just commited some critical changes, I thought were included in my initial commit, without them the fix is not really working. I do apologize as this was totally my fault.

Copy link
Copy Markdown
Contributor

@bito-code-review bito-code-review bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review Agent Run #0294b9

Actionable Suggestions - 1
  • superset-frontend/plugins/plugin-chart-echarts/src/MixedTimeseries/controlPanel.tsx - 1
    • Default label position change affects horizontal charts · Line 209-221
Additional Suggestions - 1
  • superset-frontend/plugins/plugin-chart-echarts/src/MixedTimeseries/controlPanel.tsx - 1
    • Inconsistent 'auto' support in UI vs types · Line 209-221
      Removing 'auto' from label position choices creates inconsistency, as type definitions still allow 'auto' and the code handles it (mapping to orientation-aware positions). Existing saved configs with 'auto' may still work, but users can't select it in the UI. Align choices with supported types or update types to match.
Review Details
  • Files reviewed - 3 · Commit Range: eae83bb..8abd4dd
    • superset-frontend/plugins/plugin-chart-echarts/src/MixedTimeseries/controlPanel.tsx
    • superset-frontend/plugins/plugin-chart-echarts/src/MixedTimeseries/types.ts
    • superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformers.ts
  • Files skipped - 0
  • Tools
    • Whispers (Secret Scanner) - ✔︎ Successful
    • Detect-secrets (Secret Scanner) - ✔︎ Successful
    • Eslint (Linter) - ✔︎ Successful

Bito Usage Guide

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 documentation for additional commands.

Configuration

This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.

Documentation & Help

AI Code Review powered by Bito Logo

Comment on lines +209 to +221
freeForm: false,
label: t('Label Position'),
choices: [
['top', t('Top')],
['inside', t('Inside')],
['bottom', t('Bottom')],
['left', t('Left')],
['right', t('Right')],
],
default: 'top',
renderTrigger: true,
description: t(
'Position of the data label relative to the bar segment',
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Default label position change affects horizontal charts

Changing the default label position from 'auto' to 'top' alters behavior for horizontal charts, where labels now appear at 'top' instead of 'inside' (as 'auto' would map to). This may reduce readability for horizontal bar charts. If 'auto' was removed intentionally, consider updating the default to 'inside' for horizontal orientations or confirming this UX change is intended.

Code Review Run #0294b9


Should Bito avoid suggestions like this for future reviews? (Manage Rules)

  • Yes, avoid them

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

plugins size:L This PR changes 100-499 lines, ignoring generated files size/XL

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Value is displayed on top of the stacked bar

3 participants