-
Notifications
You must be signed in to change notification settings - Fork 16.6k
fix(echarts): formula annotations not rendering with dataset-level columns label #37522
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
fix(echarts): formula annotations not rendering with dataset-level columns label #37522
Conversation
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
superset-frontend/plugins/plugin-chart-echarts/test/MixedTimeseries/transformProps.test.ts
Outdated
Show resolved
Hide resolved
superset-frontend/plugins/plugin-chart-echarts/test/MixedTimeseries/transformProps.test.ts
Outdated
Show resolved
Hide resolved
…tion in MixedTimeseries tests
…to match the other tests pattern
Code Review Agent Run #e2b581Actionable 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 |
There was a problem hiding this 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 fixes an issue where formula annotations failed to render in ECharts timeseries charts when the X-axis column had a dataset-level label. The problem occurred because the formula evaluation function was trying to access row data using the column name (e.g., "ds"), but the data structure used label keys (e.g., "Time Label") instead.
Changes:
- Resolved xAxisLabel from column name to label via verboseMap before passing it to transformFormulaAnnotation
- Added comprehensive test cases to verify formula annotations work with dataset-level labels
- Refactored test utilities to use helper functions for creating test data and chart props
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts | Added type assertions for better TypeScript compatibility; the core fix (xAxisLabel resolution) was already in place |
| superset-frontend/plugins/plugin-chart-echarts/src/MixedTimeseries/transformProps.ts | Added type assertions for better TypeScript compatibility; the core fix (xAxisLabel resolution) was already in place |
| superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/transformProps.test.ts | Added helper functions and comprehensive test cases for formula annotations with dataset-level labels, including backward compatibility tests |
| superset-frontend/plugins/plugin-chart-echarts/test/MixedTimeseries/transformProps.test.ts | Added helper functions and test case for formula annotations with dataset-level labels |
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts
Outdated
Show resolved
Hide resolved
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts
Outdated
Show resolved
Hide resolved
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts
Outdated
Show resolved
Hide resolved
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts
Outdated
Show resolved
Hide resolved
superset-frontend/plugins/plugin-chart-echarts/src/MixedTimeseries/transformProps.ts
Outdated
Show resolved
Hide resolved
superset-frontend/plugins/plugin-chart-echarts/src/MixedTimeseries/transformProps.ts
Outdated
Show resolved
Hide resolved
Code Review Agent Run #fef076Actionable 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 |
| } = datasource as typeof datasource & { currencyCodeColumn?: string }; | ||
| const queryData0 = queriesData[0] as TimeseriesChartDataResponseResult & { | ||
| detected_currency?: string | null; | ||
| }; | ||
| const queryData1 = queriesData[1] as TimeseriesChartDataResponseResult & { | ||
| detected_currency?: string | null; | ||
| }; | ||
| const { label_map: labelMap, detected_currency: backendDetectedCurrency } = | ||
| queriesData[0] as TimeseriesChartDataResponseResult; | ||
| queryData0; | ||
| const { label_map: labelMapB, detected_currency: backendDetectedCurrencyB } = | ||
| queriesData[1] as TimeseriesChartDataResponseResult; | ||
| queryData1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change doesn't seem necessary?
| } = datasource as typeof datasource & { currencyCodeColumn?: string }; | ||
| const [queryData] = queriesData; | ||
| const queryDataTyped = queryData as TimeseriesChartDataResponseResult & { | ||
| detected_currency?: string | null; | ||
| }; | ||
| const { | ||
| data = [], | ||
| label_map = {}, | ||
| detected_currency: backendDetectedCurrency, | ||
| } = queryData as TimeseriesChartDataResponseResult; | ||
| } = queryDataTyped; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same thing here.
| const { label_map: labelMap, detected_currency: backendDetectedCurrency } = | ||
| queriesData[0] as TimeseriesChartDataResponseResult; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion: Destructuring directly from queriesData[0] can throw if queriesData[0] is undefined at runtime; wrap the source in a nullish-default before destructuring and provide a safe default for label_map so downstream code doesn't access properties on undefined. [possible bug]
Severity Level: Major ⚠️
- ❌ MixedTimeseries chart render crashes (transformProps.ts).
- ⚠️ Formula/annotation processing aborted silently.
- ⚠️ Unit tests invoking transformProps may fail.| const { label_map: labelMap, detected_currency: backendDetectedCurrency } = | |
| queriesData[0] as TimeseriesChartDataResponseResult; | |
| const q0 = (queriesData[0] ?? {}) as TimeseriesChartDataResponseResult; | |
| const { label_map: labelMap = {}, detected_currency: backendDetectedCurrency } = q0; |
Steps of Reproduction ✅
1. Open file
superset-frontend/plugins/plugin-chart-echarts/src/MixedTimeseries/transformProps.ts and
locate function `transformProps` (function start around line 121). Note the destructuring
at lines 144-145 that reads `queriesData[0]` directly.
2. In a unit test or Node REPL import this function and call it directly with a minimal
chartProps object whose `queriesData` is an empty array (e.g., `{ ...minimalProps,
queriesData: [] }`). This simulates a render where no query result was provided for query
A.
3. Execution reaches transformProps and attempts the destructuring on lines 144-145.
Because `queriesData[0]` is undefined, JavaScript throws a TypeError when trying to
destructure `label_map` from undefined (error observed at transformProps.ts:144-145).
4. Note: the presence of defensive code in getAnnotationData (see
superset-frontend/plugins/plugin-chart-echarts/src/utils/annotation.ts:132-140) uses
optional chaining on `queriesData[0]`, indicating callers can provide missing query data.
Therefore the destructuring at transformProps is a real risk rather than a purely
theoretical issue.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:** 144:145
**Comment:**
*Possible Bug: Destructuring directly from `queriesData[0]` can throw if `queriesData[0]` is undefined at runtime; wrap the source in a nullish-default before destructuring and provide a safe default for `label_map` so downstream code doesn't access properties on undefined.
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.
Code Review Agent Run #9c376dActionable 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 |
SUMMARY
Issue:
Formula annotations don't render when the X-axis column has a dataset-level label; the annotation appears in the legend but the line doesn't draw.
Root Cause:
evalFormula accessed row[xAxis] using the column name (e.g., "ds"), but the data uses label keys (e.g., "Time Label"), so row[xAxis] was undefined and no annotation points were generated.
Fix:
Resolve xAxisLabel from column name to label via verboseMap and pass it to transformFormulaAnnotation, so evalFormula uses the label key that matches the data structure.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
BEFORE:
AFTER:
TESTING INSTRUCTIONS
Expected results
That annotation layer shows up in the chart.
ADDITIONAL INFORMATION