feat(explore): Add Echarts option editor#37868
Merged
justinpark merged 4 commits intoMar 4, 2026
Merged
Conversation
justinpark
requested review from
eschutho,
geido,
kgabryje,
michael-s-molina,
rusackas and
villebro
as code owners
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
![bito-code-review[bot]](https://avatars.githubusercontent.com/in/1061978?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review Agent Run #b17c16
Actionable Suggestions - 4
-
superset-frontend/packages/superset-ui-chart-controls/src/utils/echartOptionsSchema.ts - 4
- Missing ECharts text alignment properties · Line 92-95
- Missing ECharts label alignment properties · Line 169-170
- Missing ECharts axis label alignment properties · Line 335-337
- Incomplete axisPointer type enum · Line 719-719
Additional Suggestions - 11
-
superset-frontend/src/explore/components/controls/JSEditorControl.tsx - 2
-
Fix React key in map · Line 96-97Using the error message string as the React key could cause warnings if messages are duplicated; use the array index instead.
Code suggestion
@@ -96,2 +96,2 @@ - {error.validationErrors.map(err => ( - <div key={err}>{err}</div> + {error.validationErrors.map((err, index) => ( + <div key={index}>{err}</div>
-
Remove leftover comment · Line 54-54This commented line appears to be leftover code and should be removed for cleaner code.
Code suggestion
@@ -54,1 +54,0 @@ -// safeParseEChartOptions;
-
-
superset-frontend/packages/superset-ui-chart-controls/src/utils/safeEchartOptionsParser.test.ts - 6
-
Missing error type assertion · Line 85-91The test checks that parseEChartOptions throws EChartOptionsParseError for invalid values like -Infinity, but it doesn't verify the error type. Other tests in the file check errorType (e.g., 'security_error' for dangerous constructs), so add this assertion to ensure the error is validation_error as expected from schema rejection.
Code suggestion
@@ -85,7 +85,12 @@ test('parseEChartOptions throws for special numeric values like -Infinity', () => { // Special values like -Infinity are not valid JSON-serializable values const input = `{ xAxis: { min: -Infinity, splitNumber: 5 } }`; // -Infinity is not a valid value for 'min' in the schema expect(() => parseEChartOptions(input)).toThrow(EChartOptionsParseError); + try { + parseEChartOptions(input); + } catch (error) { + expect(error).toBeInstanceOf(EChartOptionsParseError); + expect((error as EChartOptionsParseError).errorType).toBe('validation_error'); + } }); -
Missing error type assertion · Line 153-157Similar to other security-related throw tests, this should check that the error type is 'security_error' since computed properties are blocked by AST validation.
Code suggestion
@@ -153,5 +153,10 @@ test('parseEChartOptions throws for computed properties', () => { const input = `{ [dynamicKey]: 'value' }`; expect(() => parseEChartOptions(input)).toThrow(EChartOptionsParseError); + try { + parseEChartOptions(input); + } catch (error) { + expect(error).toBeInstanceOf(EChartOptionsParseError); + expect((error as EChartOptionsParseError).errorType).toBe('security_error'); + } }); -
Missing error type assertion · Line 159-163Template literals with expressions are blocked by AST validation, so error type should be 'security_error' for consistency.
Code suggestion
@@ -159,5 +159,10 @@ test('parseEChartOptions throws for template literals with expressions', () => { const input = '{ text: `Hello ${name}` }'; expect(() => parseEChartOptions(input)).toThrow(EChartOptionsParseError); + try { + parseEChartOptions(input); + } catch (error) { + expect(error).toBeInstanceOf(EChartOptionsParseError); + expect((error as EChartOptionsParseError).errorType).toBe('security_error'); + } }); -
Missing error type assertion · Line 173-177New expressions are unsafe AST nodes, so error type is 'security_error'.
Code suggestion
@@ -173,5 +173,10 @@ test('parseEChartOptions throws for new expressions', () => { const input = `{ date: new Date() }`; expect(() => parseEChartOptions(input)).toThrow(EChartOptionsParseError); + try { + parseEChartOptions(input); + } catch (error) { + expect(error).toBeInstanceOf(EChartOptionsParseError); + expect((error as EChartOptionsParseError).errorType).toBe('security_error'); + } }); -
Missing error type assertion · Line 179-183Member expressions like Math.PI are blocked as unsafe, so 'security_error'.
Code suggestion
@@ -179,5 +179,10 @@ test('parseEChartOptions throws for member expressions', () => { const input = `{ value: Math.PI }`; expect(() => parseEChartOptions(input)).toThrow(EChartOptionsParseError); + try { + parseEChartOptions(input); + } catch (error) { + expect(error).toBeInstanceOf(EChartOptionsParseError); + expect((error as EChartOptionsParseError).errorType).toBe('security_error'); + } }); -
Missing test coverage · Line 354-355The test suite covers many cases but misses validation of invalid array items (e.g., series with bad objects), invalid primitive types (e.g., animation as string), empty objects, and filtering of unknown properties. Adding these would ensure the parser behaves correctly in edge cases.
-
-
superset-frontend/src/explore/components/controls/JSEditorControl.test.tsx - 2
-
Incomplete onChange assertion · Line 80-85The test verifies that onChange is called when editor content changes, but it does not check the value passed to onChange. To better assert the behavior per BITO rule [6262], consider using fireEvent.change(editor, { target: { value: '{ }' } }) and then expect(defaultProps.onChange).toHaveBeenCalledWith('{ }').
-
Misleading test name · Line 115-119The test name 'renders with description tooltip' implies testing tooltip functionality, but the assertion only checks that the label renders. The description prop is passed to ControlHeader for tooltip display, but this is not verified. Per BITO rule [6262], rename to 'renders with description prop' or add tooltip assertion if intended.
-
-
superset-frontend/plugins/plugin-chart-echarts/src/utils/mergeCustomEChartOptions.test.ts - 1
-
Misleading test name · Line 134-134The test name suggests it handles null values in custom options, but the test actually verifies that base properties not specified in custom options are preserved. This could mislead readers expecting null value handling.
Code suggestion
@@ -134,1 +134,1 @@ - test('mergeCustomEChartOptions handles null values in custom options', () => { + test('mergeCustomEChartOptions preserves base properties not present in custom options', () => {
-
Review Details
-
Files reviewed - 22 · Commit Range:
a6c6302..a6c6302- superset-frontend/packages/superset-core/src/api/contributions.ts
- superset-frontend/packages/superset-ui-chart-controls/src/shared-controls/sharedControls.tsx
- superset-frontend/packages/superset-ui-chart-controls/src/types.ts
- superset-frontend/packages/superset-ui-chart-controls/src/utils/echartOptionsSchema.ts
- superset-frontend/packages/superset-ui-chart-controls/src/utils/index.ts
- superset-frontend/packages/superset-ui-chart-controls/src/utils/safeEchartOptionsParser.test.ts
- superset-frontend/packages/superset-ui-chart-controls/src/utils/safeEchartOptionsParser.ts
- superset-frontend/packages/superset-ui-core/src/components/AsyncAceEditor/index.tsx
- superset-frontend/packages/superset-ui-core/src/components/index.ts
- superset-frontend/plugins/plugin-chart-echarts/src/MixedTimeseries/controlPanel.tsx
- superset-frontend/plugins/plugin-chart-echarts/src/MixedTimeseries/transformProps.ts
- superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/Regular/Bar/controlPanel.tsx
- superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/Regular/Line/controlPanel.tsx
- superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/Regular/SmoothLine/controlPanel.tsx
- superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts
- superset-frontend/plugins/plugin-chart-echarts/src/utils/echartOptionsTypes.ts
- superset-frontend/plugins/plugin-chart-echarts/src/utils/mergeCustomEChartOptions.test.ts
- superset-frontend/plugins/plugin-chart-echarts/src/utils/mergeCustomEChartOptions.ts
- superset-frontend/src/core/editors/AceEditorProvider.tsx
- superset-frontend/src/explore/components/controls/JSEditorControl.test.tsx
- superset-frontend/src/explore/components/controls/JSEditorControl.tsx
- superset-frontend/src/explore/components/controls/index.ts
-
Files skipped - 3
- superset-frontend/package-lock.json - Reason: Filter setting
- superset-frontend/package.json - Reason: Filter setting
- superset-frontend/packages/superset-ui-chart-controls/package.json - Reason: Filter setting
-
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
Comment on lines
+335
to
+337
| fontSize: z.number().optional(), | ||
| }) | ||
| .passthrough(); |
Contributor
There was a problem hiding this comment.
Missing ECharts axis label alignment properties
The axisLabelSchema lacks align and verticalAlign properties supported in ECharts, potentially causing validation issues for axis label alignment options.
Code suggestion
Check the AI-generated fix before applying
Suggested change
| fontSize: z.number().optional(), | |
| }) | |
| .passthrough(); | |
| fontSize: z.number().optional(), | |
| align: z.enum(['left', 'center', 'right']).optional(), | |
| verticalAlign: z.enum(['top', 'middle', 'bottom']).optional(), | |
| }) | |
| .passthrough(); |
Code Review Run #b17c16
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
| export const axisPointerSchema = z.object({ | ||
| id: z.string().optional(), | ||
| show: z.boolean().optional(), | ||
| type: z.enum(['line', 'shadow', 'none']).optional(), |
Contributor
There was a problem hiding this comment.
Incomplete axisPointer type enum
The axisPointer type enum is missing 'cross', a valid ECharts type for crosshair display. Also, add 'axis' property to support specifying target axes in tooltip configurations.
Code suggestion
Check the AI-generated fix before applying
--- a/superset-frontend/packages/superset-ui-chart-controls/src/utils/echartOptionsSchema.ts
+++ b/superset-frontend/packages/superset-ui-chart-controls/src/utils/echartOptionsSchema.ts
@@ -716,6 +716,7 @@ export const axisPointerSchema = z.object({
id: z.string().optional(),
show: z.boolean().optional(),
type: z.enum(['line', 'shadow', 'cross', 'none']).optional(),
+ axis: z.enum(['x', 'y']).optional(),
snap: z.boolean().optional(),
z: z.number().optional(),
label: z
Code Review Run #b17c16
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
justinpark
force-pushed
the
feat--add-echart-option-editor
branch
from
534793b to
8e6d4b4
Compare
Contributor
Code Review Agent Run #03329dActionable Suggestions - 0Additional Suggestions - 7
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 |
Contributor
Code Review Agent Run #e611aeActionable 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 |
kgabryje
reviewed
Feb 11, 2026
| label: t('EChart Options (JS object literals)'), | ||
| description: t( | ||
| 'A JavaScript object that adheres to the ECharts options specification. ' + | ||
| '(i.e. { title: { text: "My Chart" }, tooltip: { trigger: "item" } })', |
Member
There was a problem hiding this comment.
Can you add a link to echarts docs here?
Contributor
|
You can update the description in the echart_options control to include a link to the ECharts documentation. Here's a suggested code snippet for the change: superset-frontend/packages/superset-ui-chart-controls/src/shared-controls/sharedControls.tsx |
Member
There was a problem hiding this comment.
Thanks for the PR @justinpark.
@rusackas will probably love this as it's something he mentioned multiple times.
First-pass review:
|
|
||
| const echart_options: SharedControlConfig<'JSEditorControl'> = { | ||
| type: 'JSEditorControl', | ||
| label: t('EChart Options (JS object literals)'), |
Member
There was a problem hiding this comment.
General comment for file names, field names and descriptions: It should be ECharts not EChart.
Member
There was a problem hiding this comment.
@justinpark Echart is still being used in file names instead of ECharts
| * Reference: https://echarts.apache.org/en/option.html | ||
| */ | ||
|
|
||
| import { z } from 'zod'; |
Member
There was a problem hiding this comment.
Can you move ECharts-related files to plugins/plugin-chart-echarts? The control definition is fine but schema and parsing should leave there. You can import them using @superset-ui/plugin-chart-echarts in the editor.
michael-s-molina
changed the title
Member
|
|
Member
|
|
Member
|
For what it's worth, this seems to be where Echarts gets the "Options" data to populate their interactive Examples. At some point, we can probably use this too, to build some GUI (and templating for data insertion) around all the ECharts options. |
Member
Author
Very good point. will consider integrating schema-based autocompletion into the editor as a future improvement. |
justinpark
force-pushed
the
feat--add-echart-option-editor
branch
from
6d77bbc to
fea01eb
Compare
Contributor
There was a problem hiding this comment.
Code Review Agent Run #3194c9
Actionable Suggestions - 4
-
superset-frontend/packages/superset-ui-chart-controls/src/utils/safeEchartOptionsParser.ts - 1
- Security: Restrict bare identifiers in AST validation · Line 227-234
-
superset-frontend/packages/superset-ui-core/src/components/AsyncAceEditor/index.tsx - 1
- Incorrect default mode for JSEditor · Line 505-509
-
superset-frontend/packages/superset-ui-chart-controls/src/utils/echartOptionsSchema.ts - 1
- Missing ECharts textStyle Property · Line 84-84
-
superset-frontend/src/explore/components/controls/JSEditorControl.tsx - 1
- Error Display Logic Bug · Line 96-98
Additional Suggestions - 2
-
superset-frontend/packages/superset-ui-chart-controls/src/utils/safeEchartOptionsParser.test.ts - 1
-
Incorrect test expectation for -Infinity · Line 85-91This test expects parseEChartOptions to throw for -Infinity, but the axisSchema allows z.number() values including -Infinity. The test should be changed to verify successful parsing instead.
Code suggestion
@@ -85,7 +85,7 @@ - test('parseEChartOptions throws for special numeric values like -Infinity', () => { - // Special values like -Infinity are not valid JSON-serializable values - const input = `{ xAxis: { min: -Infinity, splitNumber: 5 }`; - - // -Infinity is not a valid value for 'min' in the schema - expect(() => parseEChartOptions(input)).toThrow(EChartOptionsParseError); - }); + test('parseEChartOptions handles special numeric values like -Infinity', () => { + // Special values like -Infinity are valid for 'min' in the schema + const input = `{ xAxis: { min: -Infinity, splitNumber: 5 } }`; + const result = parseEChartOptions(input); + + expect(result.success).toBe(true); + expect(result.data).toEqual({ xAxis: { min: -Infinity, splitNumber: 5 } }); + });
-
-
superset-frontend/packages/superset-ui-chart-controls/src/utils/echartOptionsSchema.ts - 1
-
Inaccurate Documentation · Line 823-835The comment for parseEChartOptionsStrict claims it strips invalid fields, but the implementation rejects the entire input if any field fails validation, returning an empty object. This could mislead users expecting partial validation. Consider if partial stripping is needed, or update the comment to match the strict behavior.
Code suggestion
@@ -823,5 +823,4 @@ -/** - * Validates and returns only the valid portion of the options. - * Invalid fields are stripped, not rejected. - */ -export function parseEChartOptionsStrict(data: unknown): CustomEChartOptions { +/** + * Validates and returns the validated options if successful, or an empty object if validation fails. + */ +export function parseEChartOptionsStrict(data: unknown): CustomEChartOptions {
-
Review Details
-
Files reviewed - 23 · Commit Range:
4fc89c0..fea01eb- superset-frontend/packages/superset-core/src/api/contributions.ts
- superset-frontend/packages/superset-ui-chart-controls/src/shared-controls/sharedControls.tsx
- superset-frontend/packages/superset-ui-chart-controls/src/types.ts
- superset-frontend/packages/superset-ui-chart-controls/src/utils/echartOptionsSchema.ts
- superset-frontend/packages/superset-ui-chart-controls/src/utils/index.ts
- superset-frontend/packages/superset-ui-chart-controls/src/utils/safeEchartOptionsParser.test.ts
- superset-frontend/packages/superset-ui-chart-controls/src/utils/safeEchartOptionsParser.ts
- superset-frontend/packages/superset-ui-core/src/components/AsyncAceEditor/index.tsx
- superset-frontend/packages/superset-ui-core/src/components/index.ts
- superset-frontend/plugins/plugin-chart-echarts/src/MixedTimeseries/controlPanel.tsx
- superset-frontend/plugins/plugin-chart-echarts/src/MixedTimeseries/transformProps.ts
- superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/Area/controlPanel.tsx
- superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/Regular/Bar/controlPanel.tsx
- superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/Regular/Line/controlPanel.tsx
- superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/Regular/SmoothLine/controlPanel.tsx
- superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts
- superset-frontend/plugins/plugin-chart-echarts/src/utils/echartOptionsTypes.ts
- superset-frontend/plugins/plugin-chart-echarts/src/utils/mergeCustomEChartOptions.test.ts
- superset-frontend/plugins/plugin-chart-echarts/src/utils/mergeCustomEChartOptions.ts
- superset-frontend/src/core/editors/AceEditorProvider.tsx
- superset-frontend/src/explore/components/controls/JSEditorControl.test.tsx
- superset-frontend/src/explore/components/controls/JSEditorControl.tsx
- superset-frontend/src/explore/components/controls/index.ts
-
Files skipped - 3
- superset-frontend/package-lock.json - Reason: Filter setting
- superset-frontend/package.json - Reason: Filter setting
- superset-frontend/packages/superset-ui-chart-controls/package.json - Reason: Filter setting
-
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
Comment on lines
+227
to
+234
| case 'Identifier': { | ||
| const identNode = node as Node & { name: string }; | ||
| if (DANGEROUS_IDENTIFIERS.has(identNode.name)) { | ||
| throw new Error( | ||
| `Dangerous identifier "${identNode.name}" detected at path: ${path.join('.')}`, | ||
| ); | ||
| } | ||
| break; |
Contributor
There was a problem hiding this comment.
Security: Restrict bare identifiers in AST validation
The validateNode function permits bare identifiers like 'Math' or 'foo' in value positions, which could allow non-static data and introduce security risks. Since the parser is designed for static object literals, only literal identifiers (undefined, null, true, false, NaN, Infinity) should be allowed. This gap isn't covered by existing tests and could lead to unexpected string values in parsed output.
Code Review Run #3194c9
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
Comment on lines
+505
to
+509
| export const JSEditor = AsyncAceEditor([ | ||
| 'mode/json', | ||
| 'mode/javascript', | ||
| 'theme/github', | ||
| ]); |
Contributor
There was a problem hiding this comment.
Incorrect default mode for JSEditor
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
Check the AI-generated fix before applying
Suggested change
| export const JSEditor = AsyncAceEditor([ | |
| 'mode/json', | |
| 'mode/javascript', | |
| 'theme/github', | |
| ]); | |
| export const JSEditor = AsyncAceEditor([ | |
| 'mode/javascript', | |
| 'mode/json', | |
| 'theme/github', | |
| ]); |
Code Review Run #3194c9
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
| width: numberOrPercentSchema.optional(), | ||
| height: numberOrPercentSchema.optional(), | ||
| textBorderColor: colorSchema.optional(), | ||
| textBorderWidth: z.number().optional(), |
Contributor
There was a problem hiding this comment.
Missing ECharts textStyle Property
The textStyleSchema is missing the textBorderType property, which ECharts supports for controlling text border dash styles. This could cause validation to reject valid chart configurations that use textBorderType.
Code suggestion
Check the AI-generated fix before applying
Suggested change
| textBorderWidth: z.number().optional(), | |
| textBorderWidth: z.number().optional(), | |
| textBorderType: lineTypeSchema.optional(), |
Code Review Run #3194c9
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
Comment on lines
+96
to
+98
| {error.validationErrors.map((err, idx) => ( | ||
| <div key={idx}>{err}</div> | ||
| ))} |
Contributor
There was a problem hiding this comment.
Error Display Logic Bug
The error display logic fails to show messages for parse_error and security_error types, as validationErrors is empty in those cases. This leaves users without feedback for syntax or security issues. Update to fall back to error.message when validationErrors is empty.
Code suggestion
Check the AI-generated fix before applying
Suggested change
| {error.validationErrors.map((err, idx) => ( | |
| <div key={idx}>{err}</div> | |
| ))} | |
| {error.validationErrors.length > 0 ? error.validationErrors.map((err, idx) => ( | |
| <div key={idx}>{err}</div> | |
| )) : <div>{error.message}</div>} |
Code Review Run #3194c9
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
justinpark
force-pushed
the
feat--add-echart-option-editor
branch
from
fea01eb to
7c74dd7
Compare
Contributor
There was a problem hiding this comment.
Code Review Agent Run #ee3e0d
Actionable Suggestions - 2
-
superset-frontend/packages/superset-ui-chart-controls/src/utils/echartOptionsSchema.ts - 2
- Incorrect ECharts Schema Property · Line 72-76
- Validation function comment inaccurate · Line 825-828
Additional Suggestions - 7
-
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/Regular/Line/controlPanel.tsx - 1
-
Undefined Control Section Reference · Line 219-219The added ['echart_options'] references a control section that doesn't exist in the codebase. This will likely cause the control panel to fail rendering or display incorrectly, as undefined section references aren't handled gracefully in Superset's control panel system.
-
-
superset-frontend/plugins/plugin-chart-echarts/src/MixedTimeseries/transformProps.ts - 1
-
Dead code in try block · Line 815-816The call to mergeCustomEChartOptions in the try block discards its result, making it dead code that serves no purpose. The merge is correctly handled in the subsequent const declaration.
Code suggestion
@@ -814,2 +814,1 @@ - customEchartOptions = safeParseEChartOptions(_echartOptions); - mergeCustomEChartOptions(echartOptions, customEchartOptions); + customEchartOptions = safeParseEChartOptions(_echartOptions);
-
-
superset-frontend/src/explore/components/controls/JSEditorControl.test.tsx - 3
-
Incomplete tooltip test · Line 115-119The test 'renders with description tooltip' only checks that the label renders, identical to the first test, without verifying tooltip behavior. This doesn't assert the description logic as required.
-
Incomplete error validation tests · Line 87-97The validation error tests only check for tooltip presence but not the error message content, not fully asserting the validation behavior logic.
-
Shallow onChange assertion · Line 80-85The test checks that onChange is called but not the argument, missing validation of the behavior logic for value updates.
Code suggestion
@@ -80,6 +80,6 @@ - test('calls onChange when editor content changes', async () => { - render(<JSEditorControl {...defaultProps} />); - const editor = screen.getByTestId('js-editor'); - await userEvent.type(editor, '{ }'); - expect(defaultProps.onChange).toHaveBeenCalled(); - }); + test('calls onChange when editor content changes', async () => { + render(<JSEditorControl {...defaultProps} />); + const editor = screen.getByTestId('js-editor'); + await userEvent.type(editor, '{ }'); + expect(defaultProps.onChange).toHaveBeenCalledWith('{ }'); + });
-
-
superset-frontend/packages/superset-ui-chart-controls/src/utils/echartOptionsSchema.ts - 1
-
Invalid enum value in tooltip schema · Line 382-382The tooltip trigger enum includes 'none', but ECharts docs specify only 'item' and 'axis' as valid values. 'none' is not a standard trigger option and may not behave as expected.
-
-
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts - 1
-
Dead code in options parsing · Line 851-851The call to `mergeCustomEChartOptions` on line 851 is not assigned and appears to be dead code, as the merge is performed again later. Remove it to clean up the code.
Code suggestion
@@ -850,2 +850,1 @@ - customEchartOptions = safeParseEChartOptions(_echartOptions); - mergeCustomEChartOptions(echartOptions, customEchartOptions); + customEchartOptions = safeParseEChartOptions(_echartOptions);
-
Review Details
-
Files reviewed - 23 · Commit Range:
2691a27..e435bdf- superset-frontend/packages/superset-core/src/api/contributions.ts
- superset-frontend/packages/superset-ui-chart-controls/src/shared-controls/sharedControls.tsx
- superset-frontend/packages/superset-ui-chart-controls/src/types.ts
- superset-frontend/packages/superset-ui-chart-controls/src/utils/echartOptionsSchema.ts
- superset-frontend/packages/superset-ui-chart-controls/src/utils/index.ts
- superset-frontend/packages/superset-ui-chart-controls/src/utils/safeEchartOptionsParser.test.ts
- superset-frontend/packages/superset-ui-chart-controls/src/utils/safeEchartOptionsParser.ts
- superset-frontend/packages/superset-ui-core/src/components/AsyncAceEditor/index.tsx
- superset-frontend/packages/superset-ui-core/src/components/index.ts
- superset-frontend/plugins/plugin-chart-echarts/src/MixedTimeseries/controlPanel.tsx
- superset-frontend/plugins/plugin-chart-echarts/src/MixedTimeseries/transformProps.ts
- superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/Area/controlPanel.tsx
- superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/Regular/Bar/controlPanel.tsx
- superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/Regular/Line/controlPanel.tsx
- superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/Regular/SmoothLine/controlPanel.tsx
- superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts
- superset-frontend/plugins/plugin-chart-echarts/src/utils/echartOptionsTypes.ts
- superset-frontend/plugins/plugin-chart-echarts/src/utils/mergeCustomEChartOptions.test.ts
- superset-frontend/plugins/plugin-chart-echarts/src/utils/mergeCustomEChartOptions.ts
- superset-frontend/src/core/editors/AceEditorProvider.tsx
- superset-frontend/src/explore/components/controls/JSEditorControl.test.tsx
- superset-frontend/src/explore/components/controls/JSEditorControl.tsx
- superset-frontend/src/explore/components/controls/index.ts
-
Files skipped - 3
- superset-frontend/package-lock.json - Reason: Filter setting
- superset-frontend/package.json - Reason: Filter setting
- superset-frontend/packages/superset-ui-chart-controls/package.json - Reason: Filter setting
-
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
Contributor
Code Review Agent Run #40d741Actionable 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 |
msyavuz
reviewed
Feb 18, 2026
Comment on lines
+817
to
+819
| } catch (_) { | ||
| customEchartOptions = undefined; | ||
| } |
Member
There was a problem hiding this comment.
Tagging this as the same thing Michael mentioned on other comment
justinpark
force-pushed
the
feat--add-echart-option-editor
branch
from
3f03656 to
f24207f
Compare
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| // ============================================================================= | ||
|
|
||
| export const dataZoomSchema = z.object({ | ||
| type: z.enum(['slider', 'inside']).optional(), |
Contributor
There was a problem hiding this comment.
Suggestion: The dataZoom type enum excludes the valid 'select' mode, so any user-defined custom options that configure a selection-type dataZoom will be rejected by the Zod validation and cannot be applied. [logic error]
Severity Level: Major ⚠️
- ⚠️ Selection-style dataZoom configs rejected by validation.
- ⚠️ Users cannot enable select zoom via custom options.
Suggested change
| type: z.enum(['slider', 'inside']).optional(), | |
| type: z.enum(['slider', 'inside', 'select']).optional(), |
Steps of Reproduction ✅
1. Import the validator from
`superset-frontend/packages/superset-ui-chart-controls/src/utils/echartOptionsSchema.ts`:
`import { validateEChartOptions, parseEChartOptionsStrict } from
'./echartOptionsSchema';`.
2. Call `validateEChartOptions` with a configuration that uses selection-based data zoom:
`validateEChartOptions({ dataZoom: { type: 'select' } });`.
3. The root `customEChartOptionsSchema` (lines 740–776) validates `dataZoom` using
`dataZoomSchema` from line 410 via `objectOrArray(dataZoomSchema)`.
4. Inside `dataZoomSchema`, the `type` field at line 411 is `z.enum(['slider',
'inside'])`, so `'select'` is rejected, causing `safeParse` to fail and leading
`parseEChartOptionsStrict` (lines 818–824) or any caller to drop the entire `dataZoom`
configuration (returning `{}` for invalid input).Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/packages/superset-ui-chart-controls/src/utils/echartOptionsSchema.ts
**Line:** 411:411
**Comment:**
*Logic Error: The dataZoom type enum excludes the valid 'select' mode, so any user-defined custom options that configure a selection-type dataZoom will be rejected by the Zod validation and cannot be applied.
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.| id: z.string().optional(), | ||
| show: z.boolean().optional(), | ||
| type: z.enum(['line', 'shadow', 'none']).optional(), | ||
| axis: z.enum(['x', 'y']).optional(), |
Contributor
There was a problem hiding this comment.
Suggestion: The schema for axis pointer type omits the valid 'cross' value used by ECharts, so any custom options specifying 'cross' will fail validation and be rejected by the parser, breaking legitimate configurations. [logic error]
Severity Level: Major ⚠️
- ⚠️ Valid ECharts crosshair axisPointer rejected by schema.
- ⚠️ Custom axisPointer configs lost in parseEChartOptionsStrict.
Suggested change
| axis: z.enum(['x', 'y']).optional(), | |
| type: z.enum(['line', 'shadow', 'cross', 'none']).optional(), |
Steps of Reproduction ✅
1. Import the validator from
`superset-frontend/packages/superset-ui-chart-controls/src/utils/echartOptionsSchema.ts`:
`import { validateEChartOptions } from './echartOptionsSchema';`.
2. Call `validateEChartOptions` with an options object that uses a crosshair axis pointer:
`validateEChartOptions({ axisPointer: { type: 'cross' } });`.
3. The root `customEChartOptionsSchema` (defined around lines 740–776) includes
`axisPointer: axisPointerSchema.optional()`, so validation of `axisPointer` is delegated
to `axisPointerSchema` starting at line 673.
4. Inside `axisPointerSchema` at line 676, `type` is constrained to `z.enum(['line',
'shadow', 'none'])`, so the call returns `{ success: false, error: ... }` and any wrapper
such as `parseEChartOptionsStrict` (lines 818–824) will drop the configuration and return
`{}` instead of preserving the valid `type: 'cross'` setting.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/packages/superset-ui-chart-controls/src/utils/echartOptionsSchema.ts
**Line:** 676:676
**Comment:**
*Logic Error: The schema for axis pointer type omits the valid 'cross' value used by ECharts, so any custom options specifying 'cross' will fail validation and be rejected by the parser, breaking legitimate configurations.
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.| show: z.boolean().optional(), | ||
| type: z.enum(['line', 'shadow', 'none']).optional(), | ||
| axis: z.enum(['x', 'y']).optional(), | ||
| snap: z.boolean().optional(), |
Contributor
There was a problem hiding this comment.
Suggestion: The axisPointer axis enum only allows 'x' and 'y', so valid ECharts configurations using radial axes ('radius' or 'angle') will be treated as invalid and cause parseEChartOptions to throw a validation error. [logic error]
Severity Level: Major ⚠️
- ⚠️ Polar/radial axisPointer configs rejected during validation.
- ⚠️ Custom ECharts polar charts lose axisPointer settings.
Suggested change
| snap: z.boolean().optional(), | |
| axis: z.enum(['x', 'y', 'radius', 'angle']).optional(), |
Steps of Reproduction ✅
1. Import the validator from
`superset-frontend/packages/superset-ui-chart-controls/src/utils/echartOptionsSchema.ts`:
`import { validateEChartOptions } from './echartOptionsSchema';`.
2. Call `validateEChartOptions` with options for a polar chart using a radial axis
pointer:
`validateEChartOptions({ axisPointer: { type: 'line', axis: 'radius' } });`.
3. The root schema `customEChartOptionsSchema` (lines 740–776) validates the `axisPointer`
property using `axisPointerSchema` defined at line 673.
4. In `axisPointerSchema`, the `axis` field at line 677 is `z.enum(['x', 'y'])`, which
rejects `'radius'` and `'angle'`, so validation fails and any higher-level usage
(including `parseEChartOptionsStrict` at lines 818–824 or the custom options editor that
relies on this validation) will treat the entire options object as invalid.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/packages/superset-ui-chart-controls/src/utils/echartOptionsSchema.ts
**Line:** 677:677
**Comment:**
*Logic Error: The axisPointer axis enum only allows 'x' and 'y', so valid ECharts configurations using radial axes ('radius' or 'angle') will be treated as invalid and cause parseEChartOptions to throw a validation error.
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.| const objNode = node as Node & { properties: Node[] }; | ||
| const objResult: Record<string, unknown> = {}; | ||
| objNode.properties.forEach(prop => { | ||
| const propNode = prop as Node & { key: Node; value: Node }; |
Contributor
There was a problem hiding this comment.
Suggestion: The AST security checks only block dangerous names when they appear as identifiers, but not when they are used as string/literal object keys, so a user can still supply keys like "proto", "constructor", or "prototype" in object literals; this bypasses the intended guard and can enable prototype-pollution-style attacks when these parsed options are later merged or consumed by other libraries. You should reject such dangerous property names during AST-to-value conversion so that both identifier and key-based uses are consistently blocked and produce a structured security error. [security]
Severity Level: Critical 🚨
- ❌ Parser accepts "__proto__"/constructor/prototype as string keys.
- ⚠️ Enables prototype-pollution risk when merging parsed options.
- ⚠️ Undermines secure parsing guarantees advertised in this PR.
Suggested change
| const propNode = prop as Node & { key: Node; value: Node }; | |
| const rawKey = astToValue(propNode.key); | |
| const key = String(rawKey); | |
| if (DANGEROUS_IDENTIFIERS.has(key)) { | |
| throw new EChartOptionsParseError( | |
| `Dangerous property name "${key}" is not allowed in EChart options`, | |
| 'security_error', | |
| ); | |
| } |
Steps of Reproduction ✅
1. In the frontend code, call `safeParseEChartOptions` exported from
`superset-frontend/packages/superset-ui-chart-controls/src/utils/safeEchartOptionsParser.ts:470`
with a string containing a dangerous key, for example:
`safeParseEChartOptions('{ \"series\": [], \"__proto__\": { \"polluted\": true } }');`.
2. Inside `safeParseEChartOptions`, the call is delegated to `parseEChartOptions` at
`safeEchartOptionsParser.ts:400`, which parses the wrapped input into an AST and then
calls `validateNode(expression)` (lines 413–450).
The `__proto__` key is represented as a `Literal` node inside a `Property`, not an
`Identifier`, so the `DANGEROUS_IDENTIFIERS` check in `validateNode`'s `'Identifier'`
case at `safeEchartOptionsParser.ts:227–235` is never triggered.
3. After validation, `parseEChartOptions` converts the AST back to a JavaScript value via
`astToValue(expression)` defined at `safeEchartOptionsParser.ts:310–360`.
For the outer object, the `'ObjectExpression'` case at lines 333–343 runs:
`astToValue(propNode.key)` returns the string `"__proto__"` for the `Literal` key, and
the current implementation assigns `objResult[key] = value` without any additional
security checks on `key`.
4. `parseEChartOptions` returns `{ success: true, data: { series: [], __proto__: {
polluted: true } } }` to `safeParseEChartOptions`, which forwards `result.data` to its
caller (line 473).
Any downstream code that merges this parsed options object into other objects (e.g.,
using object spread or `Object.assign`) can have its prototypes influenced by the
attacker-controlled `"__proto__"` / `"constructor"` / `"prototype"` keys, bypassing the
intended guard that only checked `DANGEROUS_IDENTIFIERS` when they appear as
`Identifier` nodes.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/packages/superset-ui-chart-controls/src/utils/safeEchartOptionsParser.ts
**Line:** 338:338
**Comment:**
*Security: The AST security checks only block dangerous names when they appear as identifiers, but not when they are used as string/literal object keys, so a user can still supply keys like "__proto__", "constructor", or "prototype" in object literals; this bypasses the intended guard and can enable prototype-pollution-style attacks when these parsed options are later merged or consumed by other libraries. You should reject such dangerous property names during AST-to-value conversion so that both identifier and key-based uses are consistently blocked and produce a structured security error.
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.
Contributor
There was a problem hiding this comment.
Code Review Agent Run #296c2d
Actionable Suggestions - 1
-
superset-frontend/packages/superset-ui-chart-controls/src/utils/echartOptionsSchema.ts - 1
- colorSchema too restrictive for gradients · Line 37-37
Additional Suggestions - 4
-
superset-frontend/src/explore/components/controls/JSEditorControl.test.tsx - 1
-
Incomplete tooltip assertion in test · Line 115-119The test 'renders with description tooltip' checks only that the label renders, but the component's description tooltip appears only when hovered=true. This doesn't verify the tooltip behavior the name suggests, potentially missing bugs in tooltip display.
-
-
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts - 1
-
Unnecessary function call · Line 852-852The code calls mergeCustomEChartOptions in the try block but discards the result, which is wasteful and confusing since the merge is done again later. This doesn't change behavior but improves clarity.
Code suggestion
@@ -849,2 +849,1 @@ - customEchartOptions = safeParseEChartOptions(_echartOptions); - mergeCustomEChartOptions(echartOptions, customEchartOptions); + customEchartOptions = safeParseEChartOptions(_echartOptions);
-
-
superset-frontend/plugins/plugin-chart-echarts/src/MixedTimeseries/transformProps.ts - 1
-
Dead code removal · Line 816-816The call to mergeCustomEChartOptions on this line doesn't assign its result, making it dead code. The merge is already handled correctly later in the function.
Code suggestion
@@ -814,5 +814,4 @@ - // that only allows static data structures (no function callbacks) - customEchartOptions = safeParseEChartOptions(_echartOptions); - mergeCustomEChartOptions(echartOptions, customEchartOptions); - } catch (_) { - customEchartOptions = undefined; + // that only allows static data structures (no function callbacks) + customEchartOptions = safeParseEChartOptions(_echartOptions); + } catch (_) { + customEchartOptions = undefined;
-
-
superset-frontend/plugins/plugin-chart-echarts/src/utils/mergeCustomEChartOptions.test.ts - 1
-
Misleading test name · Line 134-134The test name 'mergeCustomEChartOptions handles null values in custom options' is misleading because the test does not include or verify null values. Instead, it checks that properties not specified in the custom options object remain unchanged in the merged result. This could confuse developers expecting null handling. Rename to clarify the actual behavior tested.
Code suggestion
@@ -134,1 +134,1 @@ - test('mergeCustomEChartOptions handles null values in custom options', () => { + test('mergeCustomEChartOptions merges specified properties and leaves unspecified ones unchanged', () => {
-
Review Details
-
Files reviewed - 23 · Commit Range:
610e98c..f24207f- superset-frontend/packages/superset-core/src/api/contributions.ts
- superset-frontend/packages/superset-ui-chart-controls/src/shared-controls/sharedControls.tsx
- superset-frontend/packages/superset-ui-chart-controls/src/types.ts
- superset-frontend/packages/superset-ui-chart-controls/src/utils/echartOptionsSchema.ts
- superset-frontend/packages/superset-ui-chart-controls/src/utils/index.ts
- superset-frontend/packages/superset-ui-chart-controls/src/utils/safeEchartOptionsParser.test.ts
- superset-frontend/packages/superset-ui-chart-controls/src/utils/safeEchartOptionsParser.ts
- superset-frontend/packages/superset-ui-core/src/components/AsyncAceEditor/index.tsx
- superset-frontend/packages/superset-ui-core/src/components/index.ts
- superset-frontend/plugins/plugin-chart-echarts/src/MixedTimeseries/controlPanel.tsx
- superset-frontend/plugins/plugin-chart-echarts/src/MixedTimeseries/transformProps.ts
- superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/Area/controlPanel.tsx
- superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/Regular/Bar/controlPanel.tsx
- superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/Regular/Line/controlPanel.tsx
- superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/Regular/SmoothLine/controlPanel.tsx
- superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts
- superset-frontend/plugins/plugin-chart-echarts/src/utils/echartOptionsTypes.ts
- superset-frontend/plugins/plugin-chart-echarts/src/utils/mergeCustomEChartOptions.test.ts
- superset-frontend/plugins/plugin-chart-echarts/src/utils/mergeCustomEChartOptions.ts
- superset-frontend/src/core/editors/AceEditorProvider.tsx
- superset-frontend/src/explore/components/controls/JSEditorControl.test.tsx
- superset-frontend/src/explore/components/controls/JSEditorControl.tsx
- superset-frontend/src/explore/components/controls/index.ts
-
Files skipped - 3
- superset-frontend/package-lock.json - Reason: Filter setting
- superset-frontend/package.json - Reason: Filter setting
- superset-frontend/packages/superset-ui-chart-controls/package.json - Reason: Filter setting
-
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
| // ============================================================================= | ||
|
|
||
| /** Color value - hex, rgb, rgba, or named color */ | ||
| const colorSchema = z.string(); |
Contributor
There was a problem hiding this comment.
colorSchema too restrictive for gradients
colorSchema currently only allows strings, but ECharts colors support gradient objects (linear/radial) for advanced styling. This restricts valid ECharts options and could cause validation to incorrectly reject gradient colors.
Code suggestion
Check the AI-generated fix before applying
Suggested change
| const colorSchema = z.string(); | |
| const colorSchema = z.union([z.string(), z.record(z.unknown())]); |
Code Review Run #296c2d
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
Contributor
Code Review Agent Run #e854abActionable 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 |
justinpark
force-pushed
the
feat--add-echart-option-editor
branch
2 times, most recently
from
e147d80 to
c18228b
Compare
Contributor
There was a problem hiding this comment.
Code Review Agent Run #60fdb8
Actionable Suggestions - 2
-
superset-frontend/packages/superset-ui-chart-controls/src/utils/safeEchartOptionsParser.ts - 1
- Unhandled AST node type · Line 435-435
-
superset-frontend/plugins/plugin-chart-echarts/src/utils/mergeCustomEChartOptions.test.ts - 1
- Test implementation mismatch · Line 134-142
Additional Suggestions - 5
-
superset-frontend/packages/superset-ui-chart-controls/src/utils/index.ts - 1
-
Missing return type hints in exported module · Line 33-33The module being exported here includes functions lacking explicit return type hints, contravening the project's type safety standards. Adding annotations to validateEChartOptions and objectOrArray would align with the guidelines and improve maintainability.
-
-
superset-frontend/src/explore/components/controls/JSEditorControl.test.tsx - 1
-
Incomplete tooltip behavior assertion · Line 115-119The test 'renders with description tooltip' only checks that the label renders, but doesn't verify the tooltip behavior it claims to test. Tests should assert the specific logic they describe, such as hovering to display the description.
-
-
superset-frontend/packages/superset-ui-chart-controls/src/utils/safeEchartOptionsParser.ts - 1
-
JSDoc clarity issue · Line 388-388The @returns tag is worded ambiguously: 'ParseResult with either the parsed/validated data or throws EChartOptionsParseError'. This is grammatically incorrect and unclear. Rephrase to 'Returns a ParseResult on success' to align with the existing @throws tag.
Code suggestion
@@ -388,1 +388,1 @@ - * @returns ParseResult with either the parsed/validated data or throws EChartOptionsParseError + * @returns ParseResult on success
-
-
superset-frontend/packages/superset-ui-chart-controls/src/utils/echartOptionsSchema.ts - 2
-
Missing Return Type · Line 730-730Add explicit return type to objectOrArray function as per repository guidelines for improved type safety and consistency.
Code suggestion
@@ -732,1 +732,1 @@ - function objectOrArray<T extends z.ZodTypeAny>(schema: T) { + function objectOrArray<T extends z.ZodTypeAny>(schema: T): z.ZodUnion<[T, z.ZodArray<T>]> {
-
Missing Return Type · Line 810-810Add explicit return type to validateEChartOptions function as per repository guidelines for improved type safety and consistency.
Code suggestion
@@ -813,3 +813,3 @@ - export function validateEChartOptions(data: unknown) { - return customEChartOptionsSchema.safeParse(data); - } + export function validateEChartOptions(data: unknown): z.SafeParseReturnType<typeof customEChartOptionsSchema> { + return customEChartOptionsSchema.safeParse(data); + }
-
Review Details
-
Files reviewed - 23 · Commit Range:
c18228b..af698d2- superset-frontend/packages/superset-core/src/api/contributions.ts
- superset-frontend/packages/superset-ui-chart-controls/src/shared-controls/sharedControls.tsx
- superset-frontend/packages/superset-ui-chart-controls/src/types.ts
- superset-frontend/packages/superset-ui-chart-controls/src/utils/echartOptionsSchema.ts
- superset-frontend/packages/superset-ui-chart-controls/src/utils/index.ts
- superset-frontend/packages/superset-ui-chart-controls/src/utils/safeEchartOptionsParser.test.ts
- superset-frontend/packages/superset-ui-chart-controls/src/utils/safeEchartOptionsParser.ts
- superset-frontend/packages/superset-ui-core/src/components/AsyncAceEditor/index.tsx
- superset-frontend/packages/superset-ui-core/src/components/index.ts
- superset-frontend/plugins/plugin-chart-echarts/src/MixedTimeseries/controlPanel.tsx
- superset-frontend/plugins/plugin-chart-echarts/src/MixedTimeseries/transformProps.ts
- superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/Area/controlPanel.tsx
- superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/Regular/Bar/controlPanel.tsx
- superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/Regular/Line/controlPanel.tsx
- superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/Regular/SmoothLine/controlPanel.tsx
- superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts
- superset-frontend/plugins/plugin-chart-echarts/src/utils/echartOptionsTypes.ts
- superset-frontend/plugins/plugin-chart-echarts/src/utils/mergeCustomEChartOptions.test.ts
- superset-frontend/plugins/plugin-chart-echarts/src/utils/mergeCustomEChartOptions.ts
- superset-frontend/src/core/editors/AceEditorProvider.tsx
- superset-frontend/src/explore/components/controls/JSEditorControl.test.tsx
- superset-frontend/src/explore/components/controls/JSEditorControl.tsx
- superset-frontend/src/explore/components/controls/index.ts
-
Files skipped - 3
- superset-frontend/package-lock.json - Reason: Filter setting
- superset-frontend/packages/superset-ui-chart-controls/package.json - Reason: Filter setting
- superset-frontend/plugins/plugin-chart-echarts/package.json - Reason: Filter setting
-
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
| ); | ||
| } | ||
|
|
||
| const { expression } = ast.body[0]; |
Contributor
There was a problem hiding this comment.
Unhandled AST node type
The parser wraps input in parentheses for AST parsing, but doesn't handle the resulting ParenthesizedExpression node, causing all inputs to fail with 'Expected an object literal, but got: ParenthesizedExpression'. This prevents the function from working at all.
Code Review Run #60fdb8
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
Comment on lines
+134
to
+142
| test('mergeCustomEChartOptions handles null values in custom options', () => { | ||
| const base = { title: { text: 'Title' }, legend: { show: true } }; | ||
| const custom: CustomEChartOptions = { title: { text: 'New Title' } }; | ||
|
|
||
| const result = mergeCustomEChartOptions(base, custom); | ||
|
|
||
| expect(result.title).toEqual({ text: 'New Title' }); | ||
| expect(result.legend).toEqual({ show: true }); | ||
| }); |
Contributor
There was a problem hiding this comment.
Test implementation mismatch
The test named 'mergeCustomEChartOptions handles null values in custom options' assigns an object to the title property in custom options instead of null, so it doesn't verify null handling. Update the custom options to include title: null and adjust the assertion to expect(result.title).toBe(null) to match the test's purpose.
Code suggestion
Check the AI-generated fix before applying
Suggested change
| test('mergeCustomEChartOptions handles null values in custom options', () => { | |
| const base = { title: { text: 'Title' }, legend: { show: true } }; | |
| const custom: CustomEChartOptions = { title: { text: 'New Title' } }; | |
| const result = mergeCustomEChartOptions(base, custom); | |
| expect(result.title).toEqual({ text: 'New Title' }); | |
| expect(result.legend).toEqual({ show: true }); | |
| }); | |
| test('mergeCustomEChartOptions handles null values in custom options', () => { | |
| const base = { title: { text: 'Title' }, legend: { show: true } }; | |
| const custom: CustomEChartOptions = { title: null }; | |
| const result = mergeCustomEChartOptions(base, custom); | |
| expect(result.title).toBe(null); | |
| expect(result.legend).toEqual({ show: true }); | |
| }); |
Code Review Run #60fdb8
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
| height: numberOrPercentSchema.optional(), | ||
| backgroundColor: colorSchema.optional(), | ||
| borderColor: colorSchema.optional(), | ||
| borderRadius: z.number().optional(), |
Contributor
There was a problem hiding this comment.
Suggestion: The borderRadius field in the dataZoom schema only accepts a single number, but ECharts also supports the common four-corner array form (e.g., [5, 5, 0, 0]), so using the array form will fail validation and can cause the full custom options object to be treated as invalid and ignored. [logic error]
Severity Level: Major ⚠️
- ⚠️ Custom dataZoom with array border radius rejected.
- ⚠️ Users cannot style individual slider corners via editor.
Suggested change
| borderRadius: z.number().optional(), | |
| borderRadius: z.union([z.number(), z.array(z.number())]).optional(), |
Steps of Reproduction ✅
1. In
`superset-frontend/plugins/plugin-chart-echarts/src/utils/eChartOptionsSchema.ts:413-461`,
`dataZoomSchema` defines `borderRadius` as `z.number().optional()`, not allowing arrays.
2. From a caller (e.g., the new ECharts options editor), import `parseEChartOptionsStrict`
from `eChartOptionsSchema.ts:820` and call it with options containing:
`{ dataZoom: { type: 'slider', borderRadius: [5, 5, 0, 0] } }`.
3. Inside `parseEChartOptionsStrict`, `customEChartOptionsSchema.safeParse(data)`
validates `dataZoom.borderRadius`; the value `[5, 5, 0, 0]` is an array and does not
satisfy the `z.number()` schema, so `safeParse` returns `success: false`.
4. Because validation fails, `parseEChartOptionsStrict` returns `{}`
(`eChartOptionsSchema.ts:824-826`), causing all custom dataZoom styling (and any other
custom options) to be ignored when rendering the chart.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/plugins/plugin-chart-echarts/src/utils/eChartOptionsSchema.ts
**Line:** 440:440
**Comment:**
*Logic Error: The `borderRadius` field in the dataZoom schema only accepts a single number, but ECharts also supports the common four-corner array form (e.g., `[5, 5, 0, 0]`), so using the array form will fail validation and can cause the full custom options object to be treated as invalid and ignored.
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.| show: z.boolean().optional(), | ||
| type: z.enum(['line', 'shadow', 'none']).optional(), | ||
| axis: z.enum(['x', 'y']).optional(), | ||
| snap: z.boolean().optional(), |
Contributor
There was a problem hiding this comment.
Suggestion: The axis pointer schema omits the standard ECharts value 'cross' for the type field, so valid configurations using a crosshair axis pointer will fail Zod validation and can cause the entire custom options object to be rejected (e.g., parseEChartOptionsStrict returning {}), effectively dropping user-specified options. [logic error]
Severity Level: Major ⚠️
- ⚠️ Custom ECharts options with axisPointer cross are discarded.
- ⚠️ Axis crosshair pointer cannot be enabled via options editor.
Suggested change
| snap: z.boolean().optional(), | |
| type: z.enum(['line', 'shadow', 'cross', 'none']).optional(), |
Steps of Reproduction ✅
1. In
`superset-frontend/plugins/plugin-chart-echarts/src/utils/eChartOptionsSchema.ts:674-721`,
note `axisPointerSchema` defines `type` as `z.enum(['line', 'shadow', 'none'])`.
2. In any caller (e.g., the new custom ECharts options editor) import
`parseEChartOptionsStrict` from `eChartOptionsSchema.ts:820` and call it with:
`{ axisPointer: { type: 'cross' }, /* other valid options */ }`.
3. Inside `parseEChartOptionsStrict` (`eChartOptionsSchema.ts:820-826`),
`customEChartOptionsSchema.safeParse(data)` validates `axisPointer.type` against
`axisPointerSchema`; `'cross'` is not in the enum so `safeParse` returns `{ success: false
}`.
4. Because validation fails, `parseEChartOptionsStrict` returns `{}` (empty object)
instead of the provided options, so all custom options— including the intended crosshair
axis pointer—are silently dropped before chart rendering.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/plugins/plugin-chart-echarts/src/utils/eChartOptionsSchema.ts
**Line:** 677:677
**Comment:**
*Logic Error: The axis pointer schema omits the standard ECharts value `'cross'` for the `type` field, so valid configurations using a crosshair axis pointer will fail Zod validation and can cause the entire custom options object to be rejected (e.g., `parseEChartOptionsStrict` returning `{}`), effectively dropping user-specified options.
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.
Contributor
Code Review Agent Run #c8b08eActionable 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 |
justinpark
force-pushed
the
feat--add-echart-option-editor
branch
from
526e71f to
5b9a225
Compare
Contributor
There was a problem hiding this comment.
Code Review Agent Run #048e53
Actionable Suggestions - 5
-
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/Area/controlPanel.tsx - 1
- Broken custom ECharts options control · Line 254-254
-
superset-frontend/plugins/plugin-chart-echarts/src/utils/mergeCustomEChartOptions.ts - 1
- Type Safety Violation in Recursive Merge · Line 42-77
-
superset-frontend/plugins/plugin-chart-echarts/src/utils/eChartOptionsSchema.ts - 2
- Incorrect axisPointer type enum · Line 675-675
- Missing formatter in axisPointer label · Line 683-685
-
superset-frontend/src/explore/components/controls/JSEditorControl.test.tsx - 1
- Insufficient onChange test verification · Line 80-85
Additional Suggestions - 1
-
superset-frontend/src/explore/components/controls/JSEditorControl.test.tsx - 1
-
Missing test coverage for ErrorMessage display · Line 87-97The component displays validation errors in two places: ControlHeader (tested via error-tooltip) and ErrorMessage component below the editor. Only the ControlHeader errors are tested. While the current tests are sufficient, consider adding a test id to ErrorMessage for complete coverage if future tests need to verify the secondary error display.
-
Review Details
-
Files reviewed - 23 · Commit Range:
3cffa91..5b9a225- superset-frontend/packages/superset-core/src/api/editors.ts
- superset-frontend/packages/superset-ui-chart-controls/src/shared-controls/sharedControls.tsx
- superset-frontend/packages/superset-ui-chart-controls/src/types.ts
- superset-frontend/packages/superset-ui-core/src/components/AsyncAceEditor/index.tsx
- superset-frontend/packages/superset-ui-core/src/components/index.ts
- superset-frontend/plugins/plugin-chart-echarts/src/MixedTimeseries/controlPanel.tsx
- superset-frontend/plugins/plugin-chart-echarts/src/MixedTimeseries/transformProps.ts
- superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/Area/controlPanel.tsx
- superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/Regular/Bar/controlPanel.tsx
- superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/Regular/Line/controlPanel.tsx
- superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/Regular/SmoothLine/controlPanel.tsx
- superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts
- superset-frontend/plugins/plugin-chart-echarts/src/index.ts
- superset-frontend/plugins/plugin-chart-echarts/src/utils/eChartOptionsSchema.ts
- superset-frontend/plugins/plugin-chart-echarts/src/utils/echartOptionsTypes.ts
- superset-frontend/plugins/plugin-chart-echarts/src/utils/mergeCustomEChartOptions.test.ts
- superset-frontend/plugins/plugin-chart-echarts/src/utils/mergeCustomEChartOptions.ts
- superset-frontend/plugins/plugin-chart-echarts/src/utils/safeEChartOptionsParser.test.ts
- superset-frontend/plugins/plugin-chart-echarts/src/utils/safeEChartOptionsParser.ts
- superset-frontend/src/core/editors/AceEditorProvider.tsx
- superset-frontend/src/explore/components/controls/JSEditorControl.test.tsx
- superset-frontend/src/explore/components/controls/JSEditorControl.tsx
- superset-frontend/src/explore/components/controls/index.ts
-
Files skipped - 2
- superset-frontend/package-lock.json - Reason: Filter setting
- superset-frontend/plugins/plugin-chart-echarts/package.json - Reason: Filter setting
-
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
| }, | ||
| }, | ||
| ], | ||
| ['echart_options'], |
Contributor
There was a problem hiding this comment.
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)
- Yes, avoid them
Comment on lines
+42
to
+77
| export function mergeCustomEChartOptions<T extends EChartsCoreOption>( | ||
| baseOptions: T, | ||
| customOptions: CustomEChartOptions | undefined, | ||
| ): T & Partial<CustomEChartOptions> { | ||
| type MergedResult = T & Partial<CustomEChartOptions>; | ||
|
|
||
| if (!customOptions) { | ||
| return baseOptions as MergedResult; | ||
| } | ||
|
|
||
| const result = { ...baseOptions } as MergedResult; | ||
|
|
||
| for (const key of Object.keys(customOptions) as Array< | ||
| keyof typeof customOptions | ||
| >) { | ||
| const customValue = customOptions[key]; | ||
| const baseValue = result[key as keyof T]; | ||
|
|
||
| if (customValue === undefined) { | ||
| continue; | ||
| } | ||
|
|
||
| if (isPlainObject(customValue) && isPlainObject(baseValue)) { | ||
| // Recursively merge nested objects | ||
| (result as PlainObject)[key] = mergeCustomEChartOptions( | ||
| baseValue as EChartsCoreOption, | ||
| customValue as CustomEChartOptions, | ||
| ); | ||
| } else { | ||
| // Replace arrays and primitive values directly | ||
| (result as PlainObject)[key] = customValue; | ||
| } | ||
| } | ||
|
|
||
| return result; | ||
| } |
Contributor
There was a problem hiding this comment.
Type Safety Violation in Recursive Merge
Consider constraining the generic type parameter T to PlainObject and updating the recursive merge call to cast both baseValue and customValue to PlainObject. This will avoid incorrect casts to EChartsCoreOption in nested merges and improve type safety.
Code Review Run #048e53
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
| export const axisPointerSchema = z.object({ | ||
| id: z.string().optional(), | ||
| show: z.boolean().optional(), | ||
| type: z.enum(['line', 'shadow', 'none']).optional(), |
Contributor
There was a problem hiding this comment.
Incorrect axisPointer type enum
The axisPointer.type enum currently includes 'none', but ECharts documentation specifies 'line', 'shadow', or 'cross' as valid values. 'none' appears to be incorrect and should be replaced with 'cross' to match the ECharts API.
Code suggestion
Check the AI-generated fix before applying
Suggested change
| type: z.enum(['line', 'shadow', 'none']).optional(), | |
| type: z.enum(['line', 'shadow', 'cross']).optional(), |
Code Review Run #048e53
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
Comment on lines
+683
to
+685
| margin: z.number().optional(), | ||
| color: colorSchema.optional(), | ||
| fontStyle: fontStyleSchema.optional(), |
Contributor
There was a problem hiding this comment.
Missing formatter in axisPointer label
The axisPointer.label schema is missing the 'formatter' field, which ECharts supports for string-based formatting. Other label schemas in this file include formatter as z.string().optional() for consistency and to allow string formatters while excluding functions for security.
Code suggestion
Check the AI-generated fix before applying
Suggested change
| margin: z.number().optional(), | |
| color: colorSchema.optional(), | |
| fontStyle: fontStyleSchema.optional(), | |
| margin: z.number().optional(), | |
| color: colorSchema.optional(), | |
| formatter: z.string().optional(), | |
| fontStyle: fontStyleSchema.optional(), |
Code Review Run #048e53
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
Comment on lines
+80
to
+85
| test('calls onChange when editor content changes', async () => { | ||
| render(<JSEditorControl {...defaultProps} />); | ||
| const editor = screen.getByTestId('js-editor'); | ||
| await userEvent.type(editor, '{ }'); | ||
| expect(defaultProps.onChange).toHaveBeenCalled(); | ||
| }); |
Contributor
There was a problem hiding this comment.
Insufficient onChange test verification
The test checks that onChange is called when editor content changes, but doesn't verify the argument passed to it. Per BITO rule [6262], tests should verify actual business logic behavior, not just that functions are called. Since userEvent.type simulates typing '{ }' into an empty textarea, onChange should be called with '{ }'.
Code suggestion
Check the AI-generated fix before applying
Suggested change
| test('calls onChange when editor content changes', async () => { | |
| render(<JSEditorControl {...defaultProps} />); | |
| const editor = screen.getByTestId('js-editor'); | |
| await userEvent.type(editor, '{ }'); | |
| expect(defaultProps.onChange).toHaveBeenCalled(); | |
| }); | |
| test('calls onChange when editor content changes', async () => { | |
| render(<JSEditorControl {...defaultProps} />); | |
| const editor = screen.getByTestId('js-editor'); | |
| await userEvent.type(editor, '{ }'); | |
| expect(defaultProps.onChange).toHaveBeenCalled(); | |
| expect(defaultProps.onChange).toHaveBeenCalledWith('{ }'); | |
| }); |
Code Review Run #048e53
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
Contributor
Code Review Agent Run #67922bActionable 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 |
Contributor
Code Review Agent Run #34de60Actionable 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 |
michael-s-molina
approved these changes
Mar 4, 2026
3 tasks
rusackas
pushed a commit
that referenced
this pull request
Apr 17, 2026
Cover remaining 6.1 features across existing and new pages: MCP server: - Add MCP_PARSE_REQUEST_ENABLED to configuration reference - Add Audit Events section (MCP tool calls appear in Action Log) - Add Tool Pagination section documenting cursor-based pagination (#37674) Using AI with Superset: - Expand Available Tools Reference into categorized sections covering all new tools added in the MCP tool library expansion - Document preview-first workflow for generate_chart / update_chart Creating Your First Dashboard: - AG Grid server-side column filters (#35683): filter types, AND/OR logic, pagination interaction - Time Shift for AG Grid Interactive Table (#37072) - Dynamic currency formatting via currency_code_column dataset field (#36416) - ECharts option editor in Explore for JSON overrides (#37868) - Table chart: export behavior with search filter active (#36281) - Dataset folders: organizing datasets into groups (#36239) - PWA file handler: opening CSV/XLS/Parquet from OS file manager (#36191) - Share database connection option when adding a new database (#37940) Exploring Data: - Dialect-aware Format SQL (applies selected database dialect) (#39393) - SQL Lab tips section and time range natural language expressions (consolidates content from batch 4 for master branch) Importing/Exporting: - Dashboard import overwrite behavior: charts are replaced not duplicated (#36551) - UUID in REST API POST responses for dataset/chart/dashboard (#37806) New pages: - docs/docs/using-superset/embedding.mdx: embedded SDK quick start, resolvePermalinkUrl callback (#36924), DISABLE_EMBEDDED_SUPERSET_LOGOUT feature flag (#37537), URL parameters, guest token security notes - docs/admin_docs/configuration/aws-iam.mdx: cross-account IAM authentication for Aurora and Redshift via STS AssumeRole (#37585), configuration reference, trust policy setup guide Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
6 tasks
qfcwell
pushed a commit
to qfcwell/superset
that referenced
this pull request
May 12, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
SUMMARY
This commit introduces a echart option editor on basic echart types (i.e. area, bar, line, smoothline, and multi-timeseries).
This commit also introduces a secure parsing mechanism for custom EChart options, replacing the unsafe new Function() pattern with an AST-based parser. This addresses a critical security vulnerability where arbitrary JavaScript code could be executed through the EChart options input.
Key Changes:
- Uses acorn for parsing JavaScript object literals into an Abstract Syntax Tree
- Validates that only safe constructs are used (no function declarations, arrow functions, or identifiers that could execute code)
- Provides detailed error messages with line/column locations for syntax errors
- Comprehensive Zod schemas for all EChart option types (title, legend, grid, axis, tooltip, dataZoom, toolbox, visualMap, series, graphic, axisPointer)
- Type-safe validation with TypeScript type inference
- Partial validation pattern - validates each property individually to provide granular error feedback
- Structured error handling with errorType ('parse_error' | 'security_error' | 'validation_error')
- Collects all validation errors in validationErrors array
- Includes source location information for debugging
- New control component for editing EChart options
- Real-time validation with error display
- Debounced input to avoid excessive validation calls
- Updated to use safeParseEChartOptions with proper error handling
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
After:
After(on error):
TESTING INSTRUCTIONS
Create a (echart-based) chart and then set a config by echart option editor
ADDITIONAL INFORMATION