fix(chart): Show metric label in tooltips #37229
Conversation
|
CodeAnt AI is reviewing your PR. Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| labelMap, | ||
| originalVerboseMap, | ||
| ); | ||
| const verboseMap = addLabelMapToVerboseMap(labelMapB, verboseMapWithLabelMap); |
There was a problem hiding this comment.
Suggestion: Passing labelMap/labelMapB directly into addLabelMapToVerboseMap can throw if either label_map is undefined (Object.entries in the helper will receive undefined). Use a safe fallback (empty object) when calling the helper so the code won't crash when a query result omits label_map. [null pointer]
Severity Level: Critical 🚨
- ❌ MixedTimeseries charts crash at render when label_map missing.
- ⚠️ Tooltips fail to display metric labels correctly.
- ⚠️ UI shows blank chart area instead of visualization.| labelMap, | |
| originalVerboseMap, | |
| ); | |
| const verboseMap = addLabelMapToVerboseMap(labelMapB, verboseMapWithLabelMap); | |
| labelMap || {}, | |
| originalVerboseMap, | |
| ); | |
| const verboseMap = addLabelMapToVerboseMap(labelMapB || {}, verboseMapWithLabelMap); |
Steps of Reproduction ✅
1. Render a MixedTimeseries chart that uses transformProps: open the chart which triggers
transformProps in
superset-frontend/plugins/plugin-chart-echarts/src/MixedTimeseries/transformProps.ts
(function export default transformProps). The local assignment of label maps occurs at
lines 143-146 where labelMap / labelMapB are read from queriesData:
"const { label_map: labelMap } = queriesData[0] as TimeseriesChartDataResponseResult;"
and
"const { label_map: labelMapB } = queriesData[1] as
TimeseriesChartDataResponseResult;".
2. Provide query results where one of the query results omits label_map (i.e.,
queriesData[0].label_map === undefined).
This is realistic when the backend response doesn't include a label_map for a given
query.
3. transformProps then calls addLabelMapToVerboseMap at lines 148-152
("const verboseMapWithLabelMap = addLabelMapToVerboseMap(labelMap,
originalVerboseMap);"),
passing the undefined labelMap into the helper.
4. Inside the helper at
superset-frontend/plugins/plugin-chart-echarts/src/utils/forecast.ts
(addLabelMapToVerboseMap, lines ~36-62),
the implementation does Object.entries(label_map). If label_map is undefined this
throws a TypeError
("Cannot convert undefined or null to object"), causing transformProps to crash and the
chart to fail rendering.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:** 149:152
**Comment:**
*Null Pointer: Passing `labelMap`/`labelMapB` directly into `addLabelMapToVerboseMap` can throw if either `label_map` is undefined (Object.entries in the helper will receive undefined). Use a safe fallback (empty object) when calling the helper so the code won't crash when a query result omits `label_map`.
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.
superset-frontend/plugins/plugin-chart-echarts/src/utils/forecast.ts
Outdated
Show resolved
Hide resolved
|
CodeAnt AI finished reviewing your PR. |
![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 #d67806
Actionable Suggestions - 4
-
superset-frontend/plugins/plugin-chart-echarts/src/utils/forecast.ts - 1
- Logic Error in Label Replacement · Line 50-59
-
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.test.ts - 1
- Incorrect test expectation · Line 449-459
-
superset-frontend/plugins/plugin-chart-echarts/src/utils/forecast.test.ts - 1
- Insufficient test coverage for label replacements · Line 41-41
-
superset-frontend/plugins/plugin-chart-echarts/src/MixedTimeseries/transformProps.test.ts - 1
- Mismatched x-axis column in test mocks · Line 290-312
Review Details
-
Files reviewed - 6 · Commit Range:
5c97dc8..5c97dc8- superset-frontend/plugins/plugin-chart-echarts/src/MixedTimeseries/transformProps.test.ts
- superset-frontend/plugins/plugin-chart-echarts/src/MixedTimeseries/transformProps.ts
- superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.test.ts
- superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts
- superset-frontend/plugins/plugin-chart-echarts/src/utils/forecast.test.ts
- superset-frontend/plugins/plugin-chart-echarts/src/utils/forecast.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
| Object.entries(label_map).forEach(([key, labels]) => { | ||
| if (labels) { | ||
| labels | ||
| .filter(l => verboseMap[l]) | ||
| .forEach(l => { | ||
| const newKey = key.replaceAll(l, verboseMap[l]); | ||
| newVerboseMap[key] = newKey; | ||
| }); | ||
| } | ||
| }); |
There was a problem hiding this comment.
The replacement logic doesn't accumulate changes for multiple labels in a key, as each replaceAll starts from the original key. This could produce incorrect human-readable labels when multiple component labels need substitution, potentially affecting chart display names in forecast visualizations.
Code suggestion
Check the AI-generated fix before applying
| Object.entries(label_map).forEach(([key, labels]) => { | |
| if (labels) { | |
| labels | |
| .filter(l => verboseMap[l]) | |
| .forEach(l => { | |
| const newKey = key.replaceAll(l, verboseMap[l]); | |
| newVerboseMap[key] = newKey; | |
| }); | |
| } | |
| }); | |
| Object.entries(label_map).forEach(([key, labels]) => { | |
| if (labels) { | |
| let newKey = key; | |
| labels | |
| .filter(l => verboseMap[l]) | |
| .forEach(l => { | |
| newKey = newKey.replaceAll(l, verboseMap[l]); | |
| }); | |
| newVerboseMap[key] = newKey; | |
| } | |
| }); |
Code Review Run #d67806
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
| expect(result.legendData).toEqual([ | ||
| 'SUM(money_for_learning)', | ||
| 'SUM(money_for_learning), 1 day ago', | ||
| 'SUM(money_for_learning), 1 week ago', | ||
| 'SUM(money_for_learning), 1 year ago', | ||
| 'Testing count', | ||
| 'Testing count, 1 day ago', | ||
| 'Testing count, 1 week ago', | ||
| 'Testing count, 1 year ago', | ||
| 'time_start', | ||
| ]); |
There was a problem hiding this comment.
The test expects 'time_start' in legendData, but the transformProps code does not include the x-axis column in legendData for timeseries charts. This will cause the test to fail or pass incorrectly.
Code Review Run #d67806
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
| 'testing_count, 1 week ago': 'Testing count, 1 week ago', | ||
| }); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
The current test only verifies behavior when at most one label per key has a verbose mapping, but the function's logic suggests it should replace all matching labels cumulatively. This misses cases where multiple labels in the array have mappings, potentially hiding a bug in the implementation where replacements aren't accumulated properly.
Code suggestion
Check the AI-generated fix before applying
| }); | |
| it('Should handle multiple matching labels in a key', () => { | |
| const result = addLabelMapToVerboseMap( | |
| { | |
| 'testing_count and testing_count': ['testing_count'], | |
| }, | |
| { | |
| testing_count: 'Testing count', | |
| }, | |
| ); | |
| expect(result).toEqual({ | |
| testing_count: 'Testing count', | |
| 'testing_count and testing_count': 'Testing count and Testing count', | |
| }); | |
| }); | |
| it('Should handle empty label_map', () => { | |
| const result = addLabelMapToVerboseMap( | |
| {}, | |
| { | |
| testing_count: 'Testing count', | |
| }, | |
| ); | |
| expect(result).toEqual({ | |
| testing_count: 'Testing count', | |
| }); | |
| }); | |
| it('Should handle empty labels array', () => { | |
| const result = addLabelMapToVerboseMap( | |
| { | |
| 'some_key': [], | |
| }, | |
| { | |
| testing_count: 'Testing count', | |
| }, | |
| ); | |
| expect(result).toEqual({ | |
| testing_count: 'Testing count', | |
| }); | |
| }); | |
| }); |
Citations
- Rule Violated: dev-standard.mdc:22
Code Review Run #d67806
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
There was a problem hiding this comment.
I agree can we add some more testing cases?
There was a problem hiding this comment.
Yes, adding more test cases will help ensure the addLabelMapToVerboseMap function handles various scenarios correctly. Here are a few additional tests to cover multiple labels with mappings, skipping unmapped labels, and cases with no eligible labels.
superset-frontend/plugins/plugin-chart-echarts/src/utils/forecast.test.ts
it('Should handle multiple labels with mappings', () => {
const result = addLabelMapToVerboseMap(
{
'revenue_total_usd': ['revenue', 'total', 'usd'],
},
{
revenue: 'Revenue',
total: 'Total',
usd: 'USD',
},
);
expect(result).toEqual({
revenue: 'Revenue',
total: 'Total',
usd: 'USD',
'revenue_total_usd': 'Revenue Total USD',
});
});
it('Should skip labels without mappings', () => {
const result = addLabelMapToVerboseMap(
{
'test_unknown': ['test', 'unknown'],
},
{
test: 'Test',
},
);
expect(result).toEqual({
test: 'Test',
'test_unknown': 'Test unknown',
});
});
it('Should not add entry if no labels have mappings', () => {
const result = addLabelMapToVerboseMap(
{
'no_match': ['unknown1', 'unknown2'],
},
{
other: 'Other',
},
);
expect(result).toEqual({
other: 'Other',
});
});
| data: [ | ||
| { | ||
| time_start: 1533081600001, | ||
| 'SUM(money_for_learning)': 101, | ||
| 'SUM(money_for_learning), 1 day ago': null, | ||
| 'SUM(money_for_learning), 1 week ago': null, | ||
| 'SUM(money_for_learning), 1 year ago': null, | ||
| testing_count: 1, | ||
| 'testing_count, 1 day ago': null, | ||
| 'testing_count, 1 week ago': null, | ||
| 'testing_count, 1 year ago': null, | ||
| }, | ||
| { | ||
| time_start: 1533168000001, | ||
| 'SUM(money_for_learning)': 5791, | ||
| 'SUM(money_for_learning), 1 day ago': 101, | ||
| 'SUM(money_for_learning), 1 week ago': null, | ||
| 'SUM(money_for_learning), 1 year ago': null, | ||
| testing_count: 131, | ||
| 'testing_count, 1 day ago': 11, | ||
| 'testing_count, 1 week ago': null, | ||
| 'testing_count, 1 year ago': null, | ||
| }, |
There was a problem hiding this comment.
The mocked queriesData uses 'time_start' as the x-axis column key in data objects, but formData.x_axis is 'ds', causing a mismatch. This leads to incorrect series extraction where 'time_start' may be treated as a series instead of the x-axis, resulting in erroneous legend entries and potential test failures.
Code Review Run #d67806
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
superset-frontend/plugins/plugin-chart-echarts/src/MixedTimeseries/transformProps.test.ts
Outdated
Show resolved
Hide resolved
| rejected_filters: [], | ||
| }, | ||
| ] as unknown as EchartsTimeseriesChartProps['queriesData'], | ||
|
|
There was a problem hiding this comment.
Is there a way to avoid the double type assignation?
There was a problem hiding this comment.
We would need to set all required objects, which would make the mock grow significantly
superset-frontend/plugins/plugin-chart-echarts/src/MixedTimeseries/transformProps.test.ts
Show resolved
Hide resolved
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.test.ts
Show resolved
Hide resolved
superset-frontend/plugins/plugin-chart-echarts/src/utils/forecast.ts
Outdated
Show resolved
Hide resolved
reynoldmorel
force-pushed
the
fix/metric-labels-in-tooltips
branch
from
5c97dc8 to
71858b4
Compare
|
CodeAnt AI is running Incremental review Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
|
CodeAnt AI Incremental review completed. |
reynoldmorel
force-pushed
the
fix/metric-labels-in-tooltips
branch
2 times, most recently
from
2a315b0 to
84a1ba1
Compare
reynoldmorel
force-pushed
the
fix/metric-labels-in-tooltips
branch
from
84a1ba1 to
4b1e005
Compare
There was a problem hiding this comment.
Code Review Agent Run #72a151
Actionable Suggestions - 2
-
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.test.ts - 1
- Incorrect test expectations · Line 157-157
-
superset-frontend/plugins/plugin-chart-echarts/src/MixedTimeseries/transformProps.test.ts - 1
- Unexpected x-axis in expected legend · Line 286-287
Review Details
-
Files reviewed - 6 · Commit Range:
4b1e005..4b1e005- superset-frontend/plugins/plugin-chart-echarts/src/MixedTimeseries/transformProps.test.ts
- superset-frontend/plugins/plugin-chart-echarts/src/MixedTimeseries/transformProps.ts
- superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.test.ts
- superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts
- superset-frontend/plugins/plugin-chart-echarts/src/utils/forecast.test.ts
- superset-frontend/plugins/plugin-chart-echarts/src/utils/forecast.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
| datasource: '1__table', | ||
| viz_type: 'echarts_timeseries', | ||
| x_axis: 'ds', | ||
| time_compare: [], |
There was a problem hiding this comment.
The test expects 'time_start' in legendData, but transformProps does not include the x-axis in the legend—only series names. With time_compare as an empty array, only 2 series are generated ('SUM(money_for_learning)' and 'Testing count'), but expectedResult has 9 items. Update time_compare to enable time-shifted series and remove 'time_start' to match the actual legend output of 8 items.
Code Review Run #72a151
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
| 'time_start', | ||
| 'time_start', |
There was a problem hiding this comment.
The expected legend includes 'time_start', but transformProps legend data only includes series names from metrics, not the x-axis.
Code Review Run #72a151
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
reynoldmorel
force-pushed
the
fix/metric-labels-in-tooltips
branch
from
4b1e005 to
627a495
Compare
Code Review Agent Run #a8ef05Actionable Suggestions - 0Additional Suggestions - 1
Review 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 |
superset-frontend/plugins/plugin-chart-echarts/src/MixedTimeseries/transformProps.test.ts
Outdated
Show resolved
Hide resolved
| * @param replacement - The human-readable value to replace the label with | ||
| * @returns The transformed key if a valid match exists, otherwise the original key | ||
| */ | ||
| const replaceLabelIfExists = ( |
reynoldmorel
force-pushed
the
fix/metric-labels-in-tooltips
branch
from
627a495 to
d91bb89
Compare
|
CodeAnt AI is running Incremental review Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
|
CodeAnt AI Incremental review completed. |
Code Review Agent Run #646fdbActionable Suggestions - 0Additional Suggestions - 3
Review 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 |
| columnFormats = {}, | ||
| currencyFormats = {}, | ||
| currencyCodeColumn, | ||
| } = datasource; | ||
| const [queryData] = queriesData; | ||
| const { | ||
| data = [], | ||
| label_map = {}, | ||
| detected_currency: backendDetectedCurrency, | ||
| } = queryData as TimeseriesChartDataResponseResult; | ||
|
|
There was a problem hiding this comment.
Suggestion: Logic bug: the datasource property is destructured as verboseMap (camelCase) but the backend/protobuf payload uses verbose_map (snake_case) in other places; as written, when the datasource provides verbose_map the code will ignore it and originalVerboseMap will default to {}, losing the original verbose mapping. Use both keys (or prefer verbose_map) so existing verbose labels are preserved. [logic error]
Severity Level: Critical 🚨
- ❌ Timeseries tooltip labels show raw metric keys
- ❌ Forecast series tooltip human-readable labels missing
- ⚠️ Chart legend may display raw column names| // support both snake_case (backend) and camelCase (possible consumers) | |
| verbose_map: originalVerboseMapSnake = {}, | |
| verboseMap: originalVerboseMapCamel = {}, | |
| columnFormats = {}, | |
| currencyFormats = {}, | |
| currencyCodeColumn, | |
| } = datasource; | |
| const [queryData] = queriesData; | |
| const { | |
| data = [], | |
| label_map = {}, | |
| detected_currency: backendDetectedCurrency, | |
| } = queryData as TimeseriesChartDataResponseResult; | |
| // prefer snake_case payload if present, otherwise fall back to camelCase | |
| const originalVerboseMap = | |
| Object.keys(originalVerboseMapSnake).length > 0 | |
| ? originalVerboseMapSnake | |
| : originalVerboseMapCamel; |
Steps of Reproduction ✅
1. Open the Timeseries chart rendering path which calls transformProps in
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts
(function declaration at transformProps.ts:117). This function receives
chartProps including `datasource`.
2. Observe datasource destructuring at transformProps.ts:138-142 where the code
only looks for the camelCase `verboseMap` property:
"const { verboseMap: originalVerboseMap = {}, ... } = datasource;".
3. In a real backend response the human-readable mapping is provided as
snake_case `verbose_map`. When datasource only contains `verbose_map`
(and not `verboseMap`), originalVerboseMap defaults to {} at
transformProps.ts:138-142.
4. At transformProps.ts:150 the code calls
"const verboseMap = addLabelMapToVerboseMap(label_map, originalVerboseMap);"
passing the now-empty originalVerboseMap. Because the backend-provided
`verbose_map` was ignored, the combined verboseMap lacks expected
human-readable entries and tooltip/label resolution falls back to raw keys.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:** 139:149
**Comment:**
*Logic Error: Logic bug: the datasource property is destructured as `verboseMap` (camelCase) but the backend/protobuf payload uses `verbose_map` (snake_case) in other places; as written, when the datasource provides `verbose_map` the code will ignore it and `originalVerboseMap` will default to {}, losing the original verbose mapping. Use both keys (or prefer `verbose_map`) so existing verbose labels are preserved.
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.
reynoldmorel
force-pushed
the
fix/metric-labels-in-tooltips
branch
from
edcff8b to
f6db9c1
Compare
4 participants
SUMMARY
This PR improves how metric names are displayed in chart tooltips by refining the column name matching logic for forecast series. The changes enhance forecast utility functions to better handle field name translation to human-readable labels, add clearer documentation for the verbose name resolution process, and introduce a lightweight test fixture to reduce test maintenance overhead.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
BEFORE
AFTER
Screen.Recording.2026-01-18.at.2.21.25.AM.mov
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION