Skip to content

fix(mixed chart): respect truncate_metric setting for series names (#38190) - #38451

Open
MelikHajlawi wants to merge 6 commits into
apache:masterfrom
MelikHajlawi:fix/mixed-chart-truncate-metric
Open

fix(mixed chart): respect truncate_metric setting for series names (#38190)#38451
MelikHajlawi wants to merge 6 commits into
apache:masterfrom
MelikHajlawi:fix/mixed-chart-truncate-metric

Conversation

@MelikHajlawi

@MelikHajlawi MelikHajlawi commented Mar 5, 2026

Copy link
Copy Markdown
Contributor

User description

SUMMARY

Fixes #38190 – "Truncate Metric" had no effect in Mixed Chart.

Problem: The Mixed Chart ignored the truncateMetric and truncateMetricB form values, always displaying the full series name (metric + group‑by) in the legend and tooltip, regardless of the checkbox state.

Solution:

  • Added truncateMetric and truncateMetricB to the destructured form data in transformProps.ts.
  • Modified series name generation for both queries:
    • When truncateMetric (or truncateMetricB) is true and the query has at least one group‑by column, the name is built using only the group‑by values (from labelMap/labelMapB).
    • If no group‑by is present, truncation has no effect – the series name remains the metric name (optionally with query identifier). This avoids empty legend entries.
    • When false, the original logic (metric + group‑by, with optional query identifier) is used.
  • Added the missing properties to the EchartsMixedTimeseriesFormData type and set defaults to false in DEFAULT_FORM_DATA (in types.ts).
  • Incorporated feedback from CodeAnt‑AI to only truncate when a group‑by exists.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Before – Truncate Metric ON, but metric still visible in legend:
before

After – Truncate Metric ON, legend shows only group‑by values:
![after]
Screenshot 2026-03-05 at 22-00-40 test-truncate-metric

TESTING INSTRUCTIONS

  1. Create a Mixed Chart with two queries:
    • Use a dataset with a time column, a metric, and a group‑by column (e.g., birth_names with SUM(num) and gender for Query A; COUNT(*) and state for Query B).
  2. Enable "Truncate Metric" for both queries (under the Data tab in Query A and Query B sections).
  3. Click Update Chart.
    • Expected: Legend entries show only the group‑by values (e.g., male, female for Query A; state names for Query B). Tooltips also omit the metric.
  4. Disable the checkboxes and update again.
    • Expected: Full names (metric + group‑by) reappear.
  5. Edge cases to verify:
    • No group‑by (single series) – with truncation enabled, the series name should remain the metric name (not disappear).
    • Multiple metrics per query – each series truncates independently.
    • showQueryIdentifiers enabled – when truncation is on, query identifiers should not appear; when off, they appear as usual.

ADDITIONAL INFORMATION


CodeAnt-AI Description

Respect Truncate Metric settings in Mixed Chart legends and tooltips

What Changed

  • The Mixed Timeseries chart now honors the truncateMetric and truncateMetricB options: when enabled and a query has group-by columns, series names and tooltips show only the group-by values (instead of "metric, group‑by"); when no group-by exists, the metric name is kept to avoid empty legend entries.
  • truncateMetric and truncateMetricB were added to the chart form data with defaults set to false.
  • Unit tests were added to verify truncation behavior for Query A, Query B, combinations of group-by presence, and showQueryIdentifiers variations.

Impact

✅ Clearer legend entries when truncation is enabled
✅ Consistent tooltip names matching legend truncation
✅ Fewer confusing or empty legend items when no group-by exists

💡 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.

@bito-code-review

bito-code-review Bot commented Mar 5, 2026

Copy link
Copy Markdown
Contributor

Bito Automatic Review Skipped - Large PR

Bito didn't auto-review this change because the pull request exceeded the line limit. No action is needed if you didn't intend for the agent to review it. Otherwise, to manually trigger a review, type /review in a comment and save.

@codeant-ai-for-open-source codeant-ai-for-open-source Bot added the size:XXL This PR changes 1000+ lines, ignoring generated files label Mar 5, 2026
@netlify

netlify Bot commented Mar 5, 2026

Copy link
Copy Markdown

Deploy Preview for superset-docs-preview ready!

Name Link
🔨 Latest commit c69ef9f
🔍 Latest deploy log https://app.netlify.com/projects/superset-docs-preview/deploys/6a4acffd39d48f0008e15b46
😎 Deploy Preview https://deploy-preview-38451--superset-docs-preview.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.
🤖 Make changes Run an agent on this branch

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

Comment thread project_structure.txt Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

what is this file used for?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@sadpandajoe Thanks for pointing this out. The file was accidentally generated by a local tool and included in the commit. I've removed it from the branch.

Comment thread superset/static/service-worker.js Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

why are we commenting this whole file out? If this file isn't needed anymore we should just delete it.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@sadpandajoe I investigated and found that this file was originally added by an unmerged PR (#36191). It existed in my local branch, and when I built the frontend, it regenerated a larger webpack version, which I unintentionally staged. Since this file does not exist in upstream/master and is unrelated to my fix, I've removed it entirely from the PR.

@sadpandajoe

Copy link
Copy Markdown
Member

@MelikHajlawi see some new code and no tests, do the existing tests already cover the updates? do we need new tests?

@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:XXL This PR changes 1000+ lines, ignoring generated files labels Mar 6, 2026
@bito-code-review

bito-code-review Bot commented Mar 6, 2026

Copy link
Copy Markdown
Contributor

Code Review Agent Run #f65333

Actionable Suggestions - 0
Additional Suggestions - 3
  • superset-frontend/plugins/plugin-chart-echarts/src/MixedTimeseries/transformProps.ts - 2
    • Inconsistent labelMap key usage · Line 514-514
      The code inconsistently uses entryName for labelMapB access in Query B, while Query A uses seriesName (equivalent to seriesEntry). This could lead to incorrect display names and formatter keys when truncateMetricB is enabled, as labelMapB may be keyed by the processed series name rather than the raw entry name.
    • Remove debug console.log · Line 228-230
      The added console.log statements appear to be debug code that should be removed before merging, as per the repository's code standards which emphasize clean production code without debug logging.
  • superset-frontend/plugins/plugin-chart-echarts/src/MixedTimeseries/types.ts - 1
    • Remove time-specific comment · Line 141-141
      The comment '//new' is time-specific and indicates recent addition, which becomes outdated over time. Per project guidelines in AGENTS.md, comments should be timeless and remain accurate. Remove this unnecessary comment.
Review Details
  • Files reviewed - 3 · Commit Range: 0c42fd8..bde723f
    • superset-frontend/plugins/plugin-chart-echarts/src/MixedTimeseries/transformProps.ts
    • superset-frontend/plugins/plugin-chart-echarts/src/MixedTimeseries/types.ts
    • superset/static/service-worker.js
  • 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 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 12, 2026
@MelikHajlawi

Copy link
Copy Markdown
Contributor Author

@sadpandajoe Thank you for the feedback. I've now added comprehensive unit tests in transformProps.test.ts that cover the truncation behavior for both queries, including cases with/without group‑by, and when truncation is enabled/disabled. All tests are passing locally. The PR is ready for another review. Thanks!

Comment on lines +231 to +232
console.log('truncateMetric:', truncateMetric, 'truncateMetricB:', truncateMetricB);
console.log('Full formData:', formData);

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: Debug logging of full form data in chart transform code runs on every render and can expose potentially sensitive dashboard/filter payloads in browser logs while also adding unnecessary runtime overhead. Remove these logs from production path. [security]

Severity Level: Major ⚠️
- ⚠️ Full chart form payload exposed in browser console.
- ⚠️ Repeated logging adds render-path client overhead.
- ⚠️ Debug noise hinders troubleshooting real frontend issues.
Suggested change
console.log('truncateMetric:', truncateMetric, 'truncateMetricB:', truncateMetricB);
console.log('Full formData:', formData);
Steps of Reproduction ✅
1. Open any Mixed Chart; chart type is enabled in `MainPreset.ts:130-132` and uses Mixed
transform path.

2. Each render calls `transformProps` via `SuperChartCore` processing
(`SuperChartCore.tsx:47-58`).

3. During every call, `transformProps.ts:231-232` writes both truncate flags and full
`formData` to browser console.

4. `formData` includes runtime chart config payload from Explore/Dashboard context
(`transformProps` destructures `...formData` at `transformProps.ts:124-228`), so those
values are continuously exposed in client logs.

5. This is production-path logging (not test-only), unlike most plugin source files where
`console.log` does not appear (`plugins/plugin-chart-echarts/src` grep shows this file as
the runtime source instance).
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:** 231:232
**Comment:**
	*Security: Debug logging of full form data in chart transform code runs on every render and can expose potentially sensitive dashboard/filter payloads in browser logs while also adding unnecessary runtime overhead. Remove these logs from production path.

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

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

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.

Code Review Agent Run #f88980

Actionable Suggestions - 1
  • superset-frontend/plugins/plugin-chart-echarts/src/MixedTimeseries/transformProps.ts - 1
Review Details
  • Files reviewed - 2 · Commit Range: bde723f..e855301
    • superset-frontend/plugins/plugin-chart-echarts/src/MixedTimeseries/transformProps.ts
    • superset-frontend/plugins/plugin-chart-echarts/test/MixedTimeseries/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

customFormattersSecondary,
formatterSecondary,
metricsB,
labelMapB?.[seriesName]?.[0],

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.

Inconsistent labelMap key usage

The labelMapB lookup key for the formatter in Query B series uses seriesName (which includes '(1)'), but truncateMetric uses seriesEntry (without '(1)'). Since tests show label_map keys like 'boy' without '(1)', and seriesEntry matches this, using seriesName here risks undefined lookups and incorrect formatting. It looks like seriesEntry should be used consistently for labelMapB access.

Code Review Run #f88980


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

  • Yes, avoid them

@rusackas

Copy link
Copy Markdown
Member

I opened a PR with a failing test for this... it validates the issue, and if you want to cherry pick it in here after rebasing (or copy the file... whatever works) we can use it to validate that this PR solves the problem.
#40146

@apache apache deleted a comment from codeant-ai-for-open-source Bot May 15, 2026
@rusackas

Copy link
Copy Markdown
Member

@MelikHajlawi this needs some touch-ups before it's mergeable.

There's a console.log('truncateMetric'...) (and one for full formData) left in transformProps.ts around line 228 that has to come out.

The two rawSeriesA/B.forEach blocks also got de-indented to column 0 in this diff. That won't survive prettier/eslint.

Once that's cleaned up and rebased, can you confirm it passes against the failing test from #40146 so we know it actually closes #38190?

Removes stray console.log debug statements and the time-specific
//new comment flagged in review, and re-indents the reformatted
series forEach blocks via prettier.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
showQueryIdentifiers: false,
});
const transformed = transformProps(chartProps);
const series = transformed.echartOptions.series as any[];

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: Replace the any[] assertion with a specific series type (for example SeriesOption[]) so the test does not introduce any. [custom_rule]

Severity Level: Minor ⚠️

Why it matters? 🤔

The new test code introduces any[], which directly violates the no-any TypeScript rule. A specific series type should be used instead.

Fix in Cursor Fix in VSCode Claude

(Use Cmd/Ctrl + Click for best experience)

Prompt for AI Agent 🤖
This is a comment left during a code review.

**Path:** superset-frontend/plugins/plugin-chart-echarts/test/MixedTimeseries/transformProps.test.ts
**Line:** 505:505
**Comment:**
	*Custom Rule: Replace the `any[]` assertion with a specific series type (for example `SeriesOption[]`) so the test does not introduce `any`.

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

showQueryIdentifiers: false,
});
const transformed = transformProps(chartProps);
const series = transformed.echartOptions.series as any[];

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: Replace the any[] assertion with a strongly typed series array to comply with the no-any rule. [custom_rule]

Severity Level: Minor ⚠️

Why it matters? 🤔

This added test casts the series array to any[], which is exactly the prohibited pattern under the no-any rule.

Fix in Cursor Fix in VSCode Claude

(Use Cmd/Ctrl + Click for best experience)

Prompt for AI Agent 🤖
This is a comment left during a code review.

**Path:** superset-frontend/plugins/plugin-chart-echarts/test/MixedTimeseries/transformProps.test.ts
**Line:** 523:523
**Comment:**
	*Custom Rule: Replace the `any[]` assertion with a strongly typed series array to comply with the no-`any` rule.

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

showQueryIdentifiers: false,
});
const transformed = transformProps(chartProps);
const series = transformed.echartOptions.series as any[];

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: Use a specific series type for this cast rather than any[] to keep new TypeScript code strictly typed. [custom_rule]

Severity Level: Minor ⚠️

Why it matters? 🤔

The test introduces a new any[] assertion, so the suggestion correctly identifies a real violation of the no-any rule.

Fix in Cursor Fix in VSCode Claude

(Use Cmd/Ctrl + Click for best experience)

Prompt for AI Agent 🤖
This is a comment left during a code review.

**Path:** superset-frontend/plugins/plugin-chart-echarts/test/MixedTimeseries/transformProps.test.ts
**Line:** 544:544
**Comment:**
	*Custom Rule: Use a specific series type for this cast rather than `any[]` to keep new TypeScript code strictly typed.

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

showQueryIdentifiers: false,
});
const transformed = transformProps(chartProps);
const series = transformed.echartOptions.series as any[];

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: Change this any[] cast to a concrete series type so the query-B truncation test remains type-safe. [custom_rule]

Severity Level: Minor ⚠️

Why it matters? 🤔

This newly added line uses any[], which is disallowed by the no-any TypeScript rule and should be replaced with a concrete series type.

Fix in Cursor Fix in VSCode Claude

(Use Cmd/Ctrl + Click for best experience)

Prompt for AI Agent 🤖
This is a comment left during a code review.

**Path:** superset-frontend/plugins/plugin-chart-echarts/test/MixedTimeseries/transformProps.test.ts
**Line:** 558:558
**Comment:**
	*Custom Rule: Change this `any[]` cast to a concrete series type so the query-B truncation test remains type-safe.

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

Comment on lines +427 to +430
if (truncateMetric && groupby.length > 0) {
const groupbyValues = labelMap?.[seriesName] || [];
displayName =
groupbyValues.length > 0 ? groupbyValues.join(', ') : entryName;

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: When truncation is enabled, the series name is reduced to group-by values without preserving query identifiers, so enabling “Show query identifiers” no longer distinguishes Query A vs Query B. This creates ambiguous legend/tooltip labels for mixed charts and breaks the expected contract of the identifier toggle. Keep the query suffix in the truncated branch when identifiers are enabled. [incomplete implementation]

Severity Level: Major ⚠️
❌ Legends can't distinguish Query A vs Query B.
⚠️ Tooltip labels ambiguous for truncated mixed series.
⚠️ ShowQueryIdentifiers toggle ignored when truncation enabled.
Steps of Reproduction ✅
1. In a Mixed Timeseries chart, configure form data so that Query A and Query B both have
a non-empty groupby (e.g., ['gender']) and set `truncateMetric: true`, `truncateMetricB:
true`, and `showQueryIdentifiers: true` in `EchartsMixedTimeseriesFormData` (see test
setup pattern at
`superset-frontend/plugins/plugin-chart-echarts/test/MixedTimeseries/transformProps.test.ts:80-137`).

2. Render the chart so that the plugin calls `transformProps(chartProps)` defined at
`superset-frontend/plugins/plugin-chart-echarts/src/MixedTimeseries/transformProps.ts:121`,
which destructures `truncateMetric`, `truncateMetricB`, and `showQueryIdentifiers` from
the merged form data at lines `213-228`.

3. For Query A, execution enters the `rawSeriesA.forEach` loop at
`transformProps.ts:161-176`. Because `truncateMetric` is true and `groupby.length > 0`,
the branch at `transformProps.ts:168-171` runs, setting `displayName` based solely on
`labelMap?.[seriesName]` (group-by values) and ignoring `showQueryIdentifiers`. As a
result, series names become just `'boy'`, `'girl'`, etc., with no `(Query A)` suffix.

4. For Query B, a symmetric path executes in `rawSeriesB.forEach` at
`transformProps.ts:498-525`: when `truncateMetricB` is true and `groupbyB.length > 0`, the
code at `transformProps.ts:9-13` (actual lines `506-510`) sets `displayName` to group-by
values only. Even though `showQueryIdentifiers` is true, both Query A and Query B series
for the same group-by (e.g., `gender = 'boy'`) share identical legend/tooltip names like
`'boy'`, making the “Show query identifiers” toggle ineffective for truncated series.

Fix in Cursor Fix in VSCode Claude

(Use Cmd/Ctrl + Click for best experience)

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:** 427:430
**Comment:**
	*Incomplete Implementation: When truncation is enabled, the series name is reduced to group-by values without preserving query identifiers, so enabling “Show query identifiers” no longer distinguishes Query A vs Query B. This creates ambiguous legend/tooltip labels for mixed charts and breaks the expected contract of the identifier toggle. Keep the query suffix in the truncated branch when identifiers are enabled.

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

Comment on lines +506 to +510
if (truncateMetricB && groupbyB.length > 0) {
const groupbyValues =
labelMapB?.[seriesEntry] || labelMapB?.[entryName] || [];
displayName =
groupbyValues.length > 0 ? groupbyValues.join(', ') : entryName;

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 Query B truncation lookup does not use the suffixed key variant already computed for secondary-series label map access, so truncateMetricB can silently fail and fall back to full names when label-map entries are keyed with the secondary suffix. Include the same key used by secondary formatter lookup when resolving group-by labels. [incorrect variable usage]

Severity Level: Major ⚠️
❌ Query B truncation may silently fail with suffixed keys.
⚠️ Mixed charts show full metric names despite truncation.
⚠️ Users experience inconsistent truncation across secondary query.
Steps of Reproduction ✅
1. Create a Mixed Timeseries chart where Query B has a non-empty groupby (e.g., `groupbyB:
['gender']`) and truncation enabled via `truncateMetricB: true` in
`EchartsMixedTimeseriesFormData` (pattern as in tests at
`superset-frontend/plugins/plugin-chart-echarts/test/MixedTimeseries/transformProps.test.ts:81-97`).

2. Ensure the second query's `label_map` (provided through `queriesData[1]`) is keyed by
the suffixed series name used elsewhere in the transform, e.g., entries under keys like
`${seriesEntry} (1)`; this is the same key (`seriesName`) used when fetching the formatter
at `transformProps.ts:29-35` (actual lines `527-533`), confirming the code expects
suffixed keys for Query B.

3. When the chart renders, `transformProps` runs (entry at `transformProps.ts:121`). In
`rawSeriesB.forEach` at `transformProps.ts:498-525`, the truncation branch at
`transformProps.ts:9-13` (`506-510`) attempts to resolve group-by labels from
`labelMapB?.[seriesEntry] || labelMapB?.[entryName]`. Because these keys omit the `(1)`
suffix, `groupbyValues` is empty when `labelMapB` is keyed by `seriesName`, causing the
fallback `displayName = entryName` to execute and leaving the full metric-containing name
in place despite `truncateMetricB` being true.

Fix in Cursor Fix in VSCode Claude

(Use Cmd/Ctrl + Click for best experience)

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:** 506:510
**Comment:**
	*Incorrect Variable Usage: The Query B truncation lookup does not use the suffixed key variant already computed for secondary-series label map access, so `truncateMetricB` can silently fail and fall back to full names when label-map entries are keyed with the secondary suffix. Include the same key used by secondary formatter lookup when resolving group-by labels.

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

Comment on lines +141 to +142
truncateMetric: false,
truncateMetricB: false,

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: Setting truncation defaults to false here conflicts with the chart-control default for “Truncate Metric” and changes behavior for payloads that omit these fields (for example, legacy saved charts), causing transform-time defaults to disagree with control defaults. Align these defaults with the control-layer default to avoid inconsistent behavior. [logic error]

Severity Level: Major ⚠️
⚠️ Legacy mixed charts misaligned with truncation checkbox default.
⚠️ Users see full metric names despite default truncation.
⚠️ Behavior diverges between controls and transform-layer defaults.
Steps of Reproduction ✅
1. Inspect the Mixed Timeseries control panel configuration in
`superset-frontend/plugins/plugin-chart-echarts/src/MixedTimeseries/controlPanel.tsx:126-129`,
where the truncate controls are defined as `name: \`truncate_metric${controlSuffix}\`` and
configured with `...sharedControls.truncate_metric` and `default:
sharedControls.truncate_metric.default`. The shared control itself is defined at
`superset-frontend/packages/superset-ui-chart-controls/src/shared-controls/sharedControls.tsx:398-413`
with `default: true`, meaning the UI checkbox “Truncate Metric” defaults to enabled.

2. Examine the Mixed Timeseries transform defaults in
`superset-frontend/plugins/plugin-chart-echarts/src/MixedTimeseries/types.ts:80-87`, where
`DEFAULT_FORM_DATA` sets `truncateMetric: false` and `truncateMetricB: false` at lines
`82-83`. In `transformProps` (`transformProps.ts:213-228`), form data is merged via `{
...DEFAULT_FORM_DATA, ...formData }`, and `truncateMetric`/`truncateMetricB` are
destructured from that merged object.

3. For a legacy or existing chart payload that contains only the snake-case fields
(`truncate_metric`, `truncate_metric_b`) from the control layer but omits the new
camel-case `truncateMetric` and `truncateMetricB` fields, the merged form data seen by
`transformProps` will keep `truncateMetric` and `truncateMetricB` at their
DEFAULT_FORM_DATA values (`false`). As a result, when `rawSeriesA.forEach` and
`rawSeriesB.forEach` execute (see truncation branches at `transformProps.ts:168-171` and
`transformProps.ts:506-510`), the code behaves as if truncation is disabled, even though
the front-end controls default to truncation enabled, leading to inconsistent series
naming relative to the control-layer default.

Fix in Cursor Fix in VSCode Claude

(Use Cmd/Ctrl + Click for best experience)

Prompt for AI Agent 🤖
This is a comment left during a code review.

**Path:** superset-frontend/plugins/plugin-chart-echarts/src/MixedTimeseries/types.ts
**Line:** 141:142
**Comment:**
	*Logic Error: Setting truncation defaults to `false` here conflicts with the chart-control default for “Truncate Metric” and changes behavior for payloads that omit these fields (for example, legacy saved charts), causing transform-time defaults to disagree with control defaults. Align these defaults with the control-layer default to avoid inconsistent behavior.

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

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

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.

Code Review Agent Run #a8dcd4

Actionable Suggestions - 1
  • superset-frontend/plugins/plugin-chart-echarts/test/MixedTimeseries/transformProps.test.ts - 1
Additional Suggestions - 1
  • superset-frontend/plugins/plugin-chart-echarts/test/MixedTimeseries/transformProps.test.ts - 1
    • Semantic duplication of datasource · Line 159-163
      The same datasource object (`{ verboseMap: { sum__num: 'sum__num' }, columnFormats: {}, currencyFormats: {} }`) appears 8 times across the diff. Extracting to a shared constant would reduce duplication and simplify future maintenance.
Review Details
  • Files reviewed - 3 · Commit Range: 0c42fd8..c69ef9f
    • 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/test/MixedTimeseries/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

datasource: {
verboseMap: {
[timeColumnName]: timeColumnLabel,
sum__num: 'sum__num',

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.

Duplicate transformProps test assertions

Consider extracting duplicated test setup and assertion logic into shared utilities to reduce code duplication across test files. Similar patterns found in MixedTimeseries/transformProps.test.ts (lines 157-171, 204-218, 460-473) and Timeseries/transformProps.test.ts (lines 296-309, 330-341). A helper function for formula series validation would eliminate this duplication.

Code Review Run #a8dcd4


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

  • Yes, avoid them

@rusackas

Copy link
Copy Markdown
Member

@MelikHajlawi thanks for cleaning up the console.logs and indentation — the fix logic and tests look right to me now. This is still showing as conflicting, and heads up: master's transformProps.ts grew displayLabelMap/displayLabelMapB (around line 477) that your truncate branches will need to populate with the truncated name, so the rebase isn't purely mechanical. Once it's green against master I think this is mergeable.

@github-actions github-actions Bot added the requires:rebase Requires rebasing on top of current master label Aug 1, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

plugins requires:rebase Requires rebasing on top of current master size/L size:L This PR changes 100-499 lines, ignoring generated files

Projects

None yet

Development

Successfully merging this pull request may close these issues.

"Truncate Metric" has no effect in Mixed Chart

3 participants