Feat/chart options configurator: The options configuration panel is added for chart components. #1750
Conversation
…into feat/check-source-code
…into feat/chart-options-configurator
WalkthroughThis pull request expands the chart component schema in a dashboard configuration file by adding comprehensive properties and event handlers to histogram and bar chart components, while introducing a new ObjectDepthConfigurator Vue component for rendering nested configuration items with support for property binding and updates. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In
`@packages/configurator/src/object-depth-configurator/ObjectDepthConfigurator.vue`:
- Line 8: The template references an undefined variable via
:data-group-index="index" in ObjectDepthConfigurator.vue; add a prop named index
to the component's props (or define it in setup) or remove/replace the binding
with the correct existing value. Specifically, if ConfigItem consumers expect a
numeric group index, add an index prop (e.g., props: { index: { type: Number,
required: true } }) to the ObjectDepthConfigurator component and use that prop
in :data-group-index, otherwise change the attribute to use the correct existing
variable or omit it.
- Around line 27-30: The prop definition for meta in ObjectDepthConfigurator.vue
uses a default arrow function that returns undefined; update the meta prop's
default to return an empty object (e.g., change the default factory to return {}
instead of using a block with no return) so that accesses like
props.meta.properties or props.meta.widget won't throw; locate the meta prop in
the component options (meta: { type: Object, default: ... }) and replace the
default implementation with a function that returns an empty object.
- Around line 34-47: The computed property properties is mutating objects from
props.meta.properties[0].content (setting item.widget.props.modelValue) and also
accesses props.meta.widget.props?.modelValue without ensuring props.meta.widget
exists; change properties to produce a new array (use map) and deep-clone each
item before assigning modelValue so you don't mutate the original props, and
guard access to props.meta.widget (e.g., read
props.meta.widget?.props?.modelValue) when retrieving propsModelValue; update
references to the computed properties, props.meta.properties,
props.meta.widget.props.modelValue, and item.widget.props.modelValue while
making these changes.
- Around line 48-50: itemChange currently reads
props.meta.widget.props.modelValue without null checks which can throw if
meta/widget/props is undefined; update itemChange to safely access the nested
value (use optional chaining on props.meta?.widget?.props?.modelValue) and
default to an empty object before spreading, then emit the update as before
(reference itemChange and the emit('update:modelValue', ...) call).
🧹 Nitpick comments (4)
packages/configurator/src/object-depth-configurator/ObjectDepthConfigurator.vue (1)
3-6: Redundant:keyon nested element.The
:key="idx"on line 5 is redundant since the parentdivalready has:key="idx". The key is only needed on the element withv-for.Proposed fix
<div v-for="(data, idx) in properties" :key="idx" class="meta-config-item"> <config-item - :key="idx" :property="data" :data-prop-index="idx"designer-demo/public/mock/bundle.json (3)
4115-4130: Extraneouslanguageprop in SelectConfigurator.The
language: "json"prop on Line 4118 appears to be copy-paste residue fromCodeConfigurator.SelectConfiguratordoes not use alanguageprop.🧹 Suggested cleanup
"widget": { "component": "SelectConfigurator", "props": { - "language": "json", "options": [ { "label": "vertical", "value": "vertical" }, { "label": "horizontal", "value": "horizontal" } ] } },
4143-4155: Mixed line endings in defaultValue.The
defaultValueon Line 4146 uses\r\ninitially but ends with just\n, creating inconsistent line endings. This could cause display or parsing inconsistencies.🧹 Suggested fix for consistent line endings
- "defaultValue": "{ \r\n\"barWidth\": 8, \r\n\"barGap\": \"60%\"\n}", + "defaultValue": "{\n \"barWidth\": 8,\n \"barGap\": \"60%\"\n}",
4066-4103: Consider relaxingrequired: truefor optional chart features.Properties like
dataRules,dataZoom,markline,legend,label,tooltip, andlineDataNameare typically optional chart enhancements. Marking all of them asrequired: truemay force users to provide values for features they don't intend to use, reducing configuration flexibility.Consider setting
"required": falsefor genuinely optional properties.
packages/configurator/src/object-depth-configurator/ObjectDepthConfigurator.vue
Show resolved
Hide resolved
| meta: { | ||
| type: Object, | ||
| default: () => {} | ||
| } |
There was a problem hiding this comment.
Default function returns undefined instead of an empty object.
The arrow function () => {} is a function body with no return statement, so it returns undefined. This can cause null reference errors when accessing props.meta.properties or props.meta.widget.
Proposed fix
meta: {
type: Object,
- default: () => {}
+ default: () => ({})
}🤖 Prompt for AI Agents
In
`@packages/configurator/src/object-depth-configurator/ObjectDepthConfigurator.vue`
around lines 27 - 30, The prop definition for meta in
ObjectDepthConfigurator.vue uses a default arrow function that returns
undefined; update the meta prop's default to return an empty object (e.g.,
change the default factory to return {} instead of using a block with no return)
so that accesses like props.meta.properties or props.meta.widget won't throw;
locate the meta prop in the component options (meta: { type: Object, default:
... }) and replace the default implementation with a function that returns an
empty object.
| const properties = computed(() => { | ||
| const result = props.meta?.properties?.[0]?.content || [] | ||
| const propsModelValue = props.meta.widget.props?.modelValue | ||
|
|
||
| if (result.length && propsModelValue) { | ||
| result.forEach((item) => { | ||
| const modelValue = propsModelValue | ||
| const model_value_property = modelValue[item.property] | ||
| item.widget.props.modelValue = | ||
| typeof model_value_property === 'boolean' ? model_value_property : model_value_property || null | ||
| }) | ||
| } | ||
| return result | ||
| }) |
There was a problem hiding this comment.
Computed property has side effects by mutating source data.
The forEach loop mutates item.widget.props.modelValue directly on the objects from props.meta.properties[0].content. Mutating data inside a computed property is a Vue anti-pattern that can cause unpredictable reactivity behavior and affect the parent component's state.
Additionally, line 36 accesses props.meta.widget.props?.modelValue without checking if props.meta.widget exists, which can throw if meta uses the default value.
Proposed fix: deep clone items before mutation
const properties = computed(() => {
- const result = props.meta?.properties?.[0]?.content || []
- const propsModelValue = props.meta.widget.props?.modelValue
+ const content = props.meta?.properties?.[0]?.content || []
+ const propsModelValue = props.meta?.widget?.props?.modelValue
- if (result.length && propsModelValue) {
- result.forEach((item) => {
- const modelValue = propsModelValue
- const model_value_property = modelValue[item.property]
- item.widget.props.modelValue =
- typeof model_value_property === 'boolean' ? model_value_property : model_value_property || null
+ if (!content.length || !propsModelValue) {
+ return content
+ }
+
+ return content.map((item) => {
+ const model_value_property = propsModelValue[item.property]
+ return {
+ ...item,
+ widget: {
+ ...item.widget,
+ props: {
+ ...item.widget?.props,
+ modelValue:
+ typeof model_value_property === 'boolean' ? model_value_property : model_value_property || null
+ }
+ }
+ }
})
- }
- return result
})📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const properties = computed(() => { | |
| const result = props.meta?.properties?.[0]?.content || [] | |
| const propsModelValue = props.meta.widget.props?.modelValue | |
| if (result.length && propsModelValue) { | |
| result.forEach((item) => { | |
| const modelValue = propsModelValue | |
| const model_value_property = modelValue[item.property] | |
| item.widget.props.modelValue = | |
| typeof model_value_property === 'boolean' ? model_value_property : model_value_property || null | |
| }) | |
| } | |
| return result | |
| }) | |
| const properties = computed(() => { | |
| const content = props.meta?.properties?.[0]?.content || [] | |
| const propsModelValue = props.meta?.widget?.props?.modelValue | |
| if (!content.length || !propsModelValue) { | |
| return content | |
| } | |
| return content.map((item) => { | |
| const model_value_property = propsModelValue[item.property] | |
| return { | |
| ...item, | |
| widget: { | |
| ...item.widget, | |
| props: { | |
| ...item.widget?.props, | |
| modelValue: | |
| typeof model_value_property === 'boolean' ? model_value_property : model_value_property || null | |
| } | |
| } | |
| } | |
| }) | |
| }) |
🤖 Prompt for AI Agents
In
`@packages/configurator/src/object-depth-configurator/ObjectDepthConfigurator.vue`
around lines 34 - 47, The computed property properties is mutating objects from
props.meta.properties[0].content (setting item.widget.props.modelValue) and also
accesses props.meta.widget.props?.modelValue without ensuring props.meta.widget
exists; change properties to produce a new array (use map) and deep-clone each
item before assigning modelValue so you don't mutate the original props, and
guard access to props.meta.widget (e.g., read
props.meta.widget?.props?.modelValue) when retrieving propsModelValue; update
references to the computed properties, props.meta.properties,
props.meta.widget.props.modelValue, and item.widget.props.modelValue while
making these changes.
| const itemChange = (property, value) => { | ||
| emit('update:modelValue', { ...props.meta.widget.props.modelValue, [property]: value }) | ||
| } |
There was a problem hiding this comment.
Missing null check can cause runtime error.
props.meta.widget.props.modelValue is accessed without null checks. If meta, widget, or props is undefined, this will throw a runtime error when itemChange is called.
Proposed fix
const itemChange = (property, value) => {
- emit('update:modelValue', { ...props.meta.widget.props.modelValue, [property]: value })
+ const currentModelValue = props.meta?.widget?.props?.modelValue || {}
+ emit('update:modelValue', { ...currentModelValue, [property]: value })
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const itemChange = (property, value) => { | |
| emit('update:modelValue', { ...props.meta.widget.props.modelValue, [property]: value }) | |
| } | |
| const itemChange = (property, value) => { | |
| const currentModelValue = props.meta?.widget?.props?.modelValue || {} | |
| emit('update:modelValue', { ...currentModelValue, [property]: value }) | |
| } |
🤖 Prompt for AI Agents
In
`@packages/configurator/src/object-depth-configurator/ObjectDepthConfigurator.vue`
around lines 48 - 50, itemChange currently reads
props.meta.widget.props.modelValue without null checks which can throw if
meta/widget/props is undefined; update itemChange to safely access the nested
value (use optional chaining on props.meta?.widget?.props?.modelValue) and
default to an empty object before spreading, then emit the update as before
(reference itemChange and the emit('update:modelValue', ...) call).
1 participant
English | 简体中文
PR
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
Background and solution
What is the current behavior?
图表组件的option配置面板现在是作为一个整体交给用户配置，体验很不好
Issue Number: N/A
What is the new behavior?
将options里各个属性拆分成单独的配置属性由用户配置
Does this PR introduce a breaking change?
Other information
Summary by CodeRabbit
Release Notes
✏️ Tip: You can customize this high-level summary in your review settings.