-
Notifications
You must be signed in to change notification settings - Fork 17.7k
feat(explore): Add Echarts option editor #37868
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -39,6 +39,7 @@ export { | |
| AsyncAceEditor, | ||
| CssEditor, | ||
| JsonEditor, | ||
| JSEditor, | ||
| SQLEditor, | ||
| FullSQLEditor, | ||
| MarkdownEditor, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -505,6 +505,7 @@ const config: ControlPanelConfig = { | |
| }, | ||
| }, | ||
| ], | ||
| ['echart_options'], | ||
| ], | ||
| }, | ||
| ], | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -58,6 +58,7 @@ import { | |
| Refs, | ||
| } from '../types'; | ||
| import { parseAxisBound } from '../utils/controls'; | ||
| import { safeParseEChartOptions } from '../utils/safeEChartOptionsParser'; | ||
| import { | ||
| dedupSeries, | ||
| extractDataTotalValues, | ||
|
|
@@ -99,6 +100,7 @@ import { | |
| getYAxisFormatter, | ||
| } from '../utils/formatters'; | ||
| import { getMetricDisplayName } from '../utils/metricDisplayName'; | ||
| import { mergeCustomEChartOptions } from '../utils/mergeCustomEChartOptions'; | ||
|
|
||
| const getFormatter = ( | ||
| customFormatters: Record<string, ValueFormatter>, | ||
|
|
@@ -122,7 +124,7 @@ export default function transformProps( | |
| const { | ||
| width, | ||
| height, | ||
| formData, | ||
| formData: { echartOptions: _echartOptions, ...formData }, | ||
| queriesData, | ||
| hooks, | ||
| filterState, | ||
|
|
@@ -803,11 +805,25 @@ export default function transformProps( | |
| focusedSeries = seriesName; | ||
| }; | ||
|
|
||
| let customEchartOptions; | ||
| try { | ||
| // Parse custom EChart options safely using AST analysis | ||
| // This replaces the unsafe `new Function()` approach with a secure parser | ||
| // that only allows static data structures (no function callbacks) | ||
| customEchartOptions = safeParseEChartOptions(_echartOptions); | ||
| } catch (_) { | ||
| customEchartOptions = undefined; | ||
| } | ||
|
Comment on lines
+814
to
+816
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Tagging this as the same thing Michael mentioned on other comment |
||
|
|
||
| const mergedEchartOptions = customEchartOptions | ||
| ? mergeCustomEChartOptions(echartOptions, customEchartOptions) | ||
| : echartOptions; | ||
|
|
||
| return { | ||
| formData, | ||
| width, | ||
| height, | ||
| echartOptions, | ||
| echartOptions: mergedEchartOptions, | ||
| setDataMask, | ||
| emitCrossFilters, | ||
| labelMap, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -251,6 +251,7 @@ const config: ControlPanelConfig = { | |
| }, | ||
| }, | ||
| ], | ||
| ['echart_options'], | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Broken custom ECharts options control
The added 'echart_options' control allows users to input custom ECharts options for Area charts, matching the implementation in other timeseries chart types. However, due to a bug in transformProps.ts where the destructuring accesses 'echartOptions' instead of 'echart_options', the custom options will not be applied. This affects all charts using this control, not just Area. Code Review Run #048e53 Should Bito avoid suggestions like this for future reviews? (Manage Rules)
|
||
| ], | ||
| }, | ||
| ], | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -216,6 +216,7 @@ const config: ControlPanelConfig = { | |
| }, | ||
| }, | ||
| ], | ||
| ['echart_options'], | ||
| ], | ||
| }, | ||
| ], | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -183,6 +183,7 @@ const config: ControlPanelConfig = { | |
| }, | ||
| }, | ||
| ], | ||
| ['echart_options'], | ||
| ], | ||
| }, | ||
| ], | ||
|
|
||
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.
The JSEditor is configured with 'mode/json' first, causing the default mode to be 'json', but given the name 'JSEditor', it appears intended for JavaScript editing. Consider reordering the modes to start with 'mode/javascript' for correct default behavior, similar to how other editors prioritize their primary mode.
Code suggestion
Code Review Run #3194c9
Should Bito avoid suggestions like this for future reviews? (Manage Rules)