fix(plugin-chart-echarts): honor per-metric formats in Timeseries tooltips - #43113
fix(plugin-chart-echarts): honor per-metric formats in Timeseries tooltips#43113WhoamiI00 wants to merge 1 commit into
Conversation
dosubot
Bot
added
viz:charts:timeseries
Code Review Agent Run #12a4d6Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| : (getCustomFormatter( | ||
| customFormatters, | ||
| metrics, | ||
| labelMap?.[seriesKey]?.[0], | ||
| ) ?? defaultFormatter); |
There was a problem hiding this comment.
Suggestion: The tooltip key is the rendered series name, but labelMap is keyed by the raw query series name. When verboseMap renames a metric, or when a time-comparison/dimension suffix is present, this lookup returns undefined and the tooltip falls back to defaultFormatter, so the per-metric format is still lost for those series. Resolve the key through the raw/display label mapping (including the existing inverted verbose map) before calling getCustomFormatter, as the mixed-timeseries tooltip does. [api mismatch]
Severity Level: Major ⚠️
- ⚠️ Verbose metric tooltips lose configured per-metric formats.
- ⚠️ Time-comparison tooltip rows can use chart-level formatting.
- ⚠️ Currency and percentage values may display misleadingly.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts
**Line:** 1428:1432
**Comment:**
*Api Mismatch: The tooltip key is the rendered series name, but `labelMap` is keyed by the raw query series name. When `verboseMap` renames a metric, or when a time-comparison/dimension suffix is present, this lookup returns `undefined` and the tooltip falls back to `defaultFormatter`, so the per-metric format is still lost for those series. Resolve the key through the raw/display label mapping (including the existing inverted verbose map) before calling `getCustomFormatter`, as the mixed-timeseries tooltip does.
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 fixThere was a problem hiding this comment.
Already addressed — this comment is against 6d72e86, the first commit. fc27dd8 (pushed a few minutes later) adds exactly the fallback suggested here:
labelMap?.[seriesKey]?.[0] ?? inverted[seriesKey],so a series renamed by verbose_name now resolves through the verbose-name inversion, matching MixedTimeseries. There's a regression test for it.
On the time-comparison case: those series are already covered without the fallback, because labelMap shifts the offset off the front of the label_map entry, leaving the metric label first. I added a third test pinning that down rather than changing the lookup.
There was a problem hiding this comment.
Good catch, and confirmed — that was a real gap. Fixed in 41f91d3.
Reproduced it first: a currency metric with comparison_type: Percentage and a 1 week ago offset rendered the derived row as $ 0.25 in the tooltip. The per-series lookup resolved that row back to the source metric, so it picked up the metric's saved CurrencyFormatter even though the value is a ratio.
Those rows now take the percent formatter instead, so the same fixture renders $ 100 for the source metric and 25.00% for the comparison row. The offsets come from rawFormData.time_compare, and the labels follow renameOperator — the offset alone, or <metric>, <offset> when several metrics are plotted.
Added the fixture you asked for; it fails without the change with Expected substring: "25.00%".
One thing I left alone deliberately: Ratio is unitless too, so it has the same problem, but the right output there is a plain number rather than a percent, and that felt like your call rather than mine. Happy to fold it in if you want it in this PR.
Also rebuilt the branch on current master — it had gone conflicting against the new tooltip-truncation tests, which sit exactly where these tests were appended. Timeseries + MixedTimeseries are green at 309 tests.
|
The flagged issue is correct. The current implementation uses a single To resolve this, you should map the rendered series name back to the raw metric name (using the superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts |
|
Good catch on the rendered-vs-raw series name — that gap was real. Pushed 8be403b: A metric with a The lookup now falls back to the verbose-name inversion (
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #43113 +/- ##
=======================================
Coverage 78.85% 78.85%
=======================================
Files 2876 2876
Lines 164581 164593 +12
Branches 38011 38020 +9
=======================================
+ Hits 129786 129796 +10
- Misses 32348 32350 +2
Partials 2447 2447
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
8be403b to
fc27dd8
Compare
Code Review Agent Run #c199a2Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
| // `labelMap`, whose values lead with the raw metric label. Series | ||
| // renamed by a verbose_name are absent from that map, so fall back to | ||
| // the verbose-name inversion, as MixedTimeseries does. | ||
| const getSeriesFormatter = (seriesKey: string) => |
There was a problem hiding this comment.
A Time Comparison set to Percentage Change produces a derived percentage, but this now selects the source metric's saved CurrencyFormatter for that row, so the reported bar-chart case can still display dollars. Could the percentage-comparison path bypass per-metric currency formatting and add that fixture?
487b347 to
41f91d3
Compare
Code Review Agent Run #971bd5Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
|
| Language | Invalidated translations |
|---|---|
es |
13 |
How to fix
1. Install dependencies (if not already set up):
pip install -r superset/translations/requirements.txt
sudo apt-get install gettext # or: brew install gettext2. Re-extract strings and sync .po files:
./scripts/translations/babel_update.shThis rewrites superset/translations/messages.pot from the current source files and merges the changes into every .po file. Strings whose msgid changed will be marked #, fuzzy.
3. Resolve the fuzzy entries in the affected language files (es):
grep -n '#, fuzzy' superset/translations/<lang>/LC_MESSAGES/messages.poFor each fuzzy entry, either rewrite the msgstr to match the new string and remove the #, fuzzy line, or clear the msgstr to "" if you cannot provide a translation.
4. Commit your changes to the .po files.
…ltips The tooltip resolved one formatter for the whole chart, so on a multi-metric chart every row rendered with the y-axis/currency format while the series labels already honored each metric's own format. Resolve the formatter per series through the same labelMap lookup the labels use. The tooltip key is the rendered series name, so a metric renamed by a verbose_name is absent from labelMap; fall back to the verbose-name inversion as MixedTimeseries does. A Percentage time comparison replaces the derived row's values with a ratio, so that row takes the percent formatter rather than the source metric's currency format. The total row keeps the chart-level formatter, since it aggregates across every series. Four regression tests cover the plain multi-metric case, verbose_name series, time-shifted series, and the Percentage comparison row.
41f91d3 to
7c15686
Compare
|
Rebuilt on current
|
![bito-code-review[bot]](https://avatars.githubusercontent.com/in/1061978?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review Agent Run #fdec25
Actionable Suggestions - 1
-
superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/transformProps.test.ts - 1
- Duplicated test setup code · Line 2640-2640
Review Details
-
Files reviewed - 2 · Commit Range:
7c15686..7c15686- superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts
- 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 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 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
| queriesData: [ | ||
| createTestQueryData( | ||
| [{ count: 1000, pct_change: 0.1234, __timestamp: BASE_TIMESTAMP }], | ||
| { label_map: { count: ['count'], pct_change: ['pct_change'] } }, |
There was a problem hiding this comment.
Syntactic duplication detected in test file. Code snippets at lines 2640-2656 and 2735-2751 share 17 lines of identical syntax. Additionally, code at lines 2628-2644 and 2666-2682 share similar patterns. Consider extracting the duplicated chartProps setup into a reusable helper function to eliminate redundancy and improve maintainability.
Code Review Run #fdec25
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
2 participants
SUMMARY
Fixes #33757.
Tooltips on
echarts_timeseries_*charts render every series with the chart-level y-axis/currency format instead of each metric's own format. The reported symptom is a metric configured as a percentage showing up in the tooltip with the currency format, while the "Show Values" label on that same series formats correctly.Root cause
getCustomFormatter(customFormatters, metrics, key?)only resolves a per-metric formatter when it is given the series key — for a chart with more than one saved metric it returnsundefinedwithout one:The series-label path passes that key; the tooltip path did not:
Timeseries/transformProps.ts)labelMap?.[seriesName]?.[0]Timeseries/transformProps.ts)defaultFormatterdefaultFormatteris built fromyAxisFormatplus the chart-level currency, so on any multi-metric chart every tooltip row was formatted with the y-axis/currency format — the behaviour described in the issue. It also covers the second report in that thread, where a currency stayed applied to a "Percentage Change" time-comparison series on a bar chart: time-shifted rows now resolve to their underlying metric's format too (third test below).MixedTimeseriesalready resolves its tooltip formatters per series key (seeMixedTimeseries/transformProps.ts, whereformatterKeyis derived per row before callinggetFormatter). This change bringsTimeseriesin line with that existing pattern.Fix
Resolve the value formatter per series inside the tooltip row loop, reusing the same
labelMaplookup the series labels already use. The total row keeps the chart-level formatter, since it aggregates across every series and so has no single metric format.The tooltip key is the rendered series name, so a metric with a
verbose_nameis absent fromlabelMap(which is keyed by the rawlabel_mapnames) and would still miss. The lookup falls back to the verbose-name inversion for those series, matchingMixedTimeseriesand the existinginverted[...]resolution already used elsewhere in this file. Both cases are covered by tests.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Chart with two saved metrics —
count(currencyUSD) andpct_change(D3 format.2%). Values are the rendered tooltip cells from the regression test added in this PR:count1k$ 1kpct_change0.123412.34%TESTING INSTRUCTIONS
Automated — three regression tests are included, covering a plain multi-metric chart, metrics carrying a
verbose_name, and a time-comparison (time-shifted) series. The first two fail onmaster; the third pins down the time-shift path, which the offset-shiftedlabelMapalready resolves correctly:cd superset-frontend npx jest plugins/plugin-chart-echarts/test/Timeseries/transformProps.test.tstransformProps.test.tspasses 71/71, and the widerTimeseries+MixedTimeseriessuites pass 294/294 across 17 suites.Manual:
.2%D3 format and the other a currency format.ADDITIONAL INFORMATION