Fix dashboard stories schema and SchemaRenderer prop spreading#286
Fix dashboard stories schema and SchemaRenderer prop spreading#286
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Co-authored-by: hotlong <50353452+hotlong@users.noreply.github.com>
|
@copilot page story error: Error: Minified React error #31; visit https://react.dev/errors/31?args[]=object%20with%20keys%20%7Btype%2C%20cols%2C%20gap%2C%20children%7D for the full message or use the non-minified dev environment for full errors and additional helpful warnings. |
Co-authored-by: hotlong <50353452+hotlong@users.noreply.github.com>
Fixed in commit 7f6148e. The error was caused by spreading schema metadata properties like Page Story - Now Working: Dashboard Story - Still Working: Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
There was a problem hiding this comment.
Pull request overview
This pull request fixes two issues that were causing the Storybook dashboard component to display as blank:
- Dashboard Schema Structure: Updated the dashboard stories to use the correct
widgetsarray structure withidandcomponentproperties, matching theDashboardSchematype definition - SchemaRenderer Prop Spreading: Modified the SchemaRenderer to spread schema properties (like
label,value) directly to components, in addition to explicitpropsobject
Changes:
- Fixed dashboard story schemas to use
widgetsarray with proper nested structure (id,title,component) - Added prop spreading in SchemaRenderer to pass schema properties directly to components
- Filtered out schema metadata properties to prevent them from being passed as React props
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| packages/react/src/SchemaRenderer.tsx | Added destructuring to filter out schema metadata properties and spread remaining properties to components, enabling direct prop access (e.g., label, value) |
| packages/components/src/stories-json/dashboard.stories.tsx | Corrected schema structure from flat children array to nested widgets array with id and component properties per DashboardSchema type |
Comments suppressed due to low confidence (1)
packages/react/src/SchemaRenderer.tsx:82
- The changes to SchemaRenderer's prop spreading behavior are significant and affect how all components receive their props. However, there don't appear to be any tests for the SchemaRenderer component itself (only LazyPluginLoader has tests in the tests directory).
According to Rule #2 (Documentation Driven Development) from the coding guidelines, when implementing or refactoring features, both code AND tests should be updated. This prop spreading change is a core behavior change that should be covered by tests to:
- Verify that schema properties like
labelandvalueare correctly passed to components - Ensure metadata properties are properly filtered out
- Validate the precedence order of props (componentProps < evaluatedSchema.props < explicit props)
- Prevent regression if this behavior needs to change in the future
Consider adding test cases for SchemaRenderer that cover these scenarios.
// Extract schema metadata properties that should NOT be passed as React props
const {
type: _type,
children: _children,
body: _body,
schema: _schema,
visible: _visible,
visibleOn: _visibleOn,
hidden: _hidden,
hiddenOn: _hiddenOn,
disabled: _disabled,
disabledOn: _disabledOn,
...componentProps
} = evaluatedSchema;
return React.createElement(Component, {
schema: evaluatedSchema,
...componentProps, // Spread non-metadata schema properties as props
...(evaluatedSchema.props || {}), // Override with explicit props if provided
className: evaluatedSchema.className,
'data-obj-id': evaluatedSchema.id,
'data-obj-type': evaluatedSchema.type,
...props
});
| const { | ||
| type: _type, | ||
| children: _children, | ||
| body: _body, | ||
| schema: _schema, | ||
| visible: _visible, | ||
| visibleOn: _visibleOn, | ||
| hidden: _hidden, | ||
| hiddenOn: _hiddenOn, | ||
| disabled: _disabled, | ||
| disabledOn: _disabledOn, | ||
| ...componentProps | ||
| } = evaluatedSchema; | ||
|
|
||
| return React.createElement(Component, { | ||
| schema: evaluatedSchema, | ||
| ...(evaluatedSchema.props || {}), | ||
| ...componentProps, // Spread non-metadata schema properties as props | ||
| ...(evaluatedSchema.props || {}), // Override with explicit props if provided | ||
| className: evaluatedSchema.className, | ||
| 'data-obj-id': evaluatedSchema.id, |
There was a problem hiding this comment.
The id property is not being filtered out in the destructuring (lines 60-71), which means it will be included in componentProps and spread on line 76. However, the schema's id is already being passed explicitly as data-obj-id on line 79.
This could cause issues:
- Components might receive both
idanddata-obj-idprops - The
idprop might be used as the DOM element'sidattribute, which could cause duplicate IDs if multiple instances of the same schema are rendered - It's unclear which
idvalue should take precedence if a component explicitly uses theidprop
Consider adding id to the exclusion list:
const {
type: _type,
id: _id, // Add this
children: _children,
...
The schema's id should only be used for the data-obj-id attribute (line 79), not passed as a direct prop to components.
| ...componentProps, // Spread non-metadata schema properties as props | ||
| ...(evaluatedSchema.props || {}), // Override with explicit props if provided | ||
| className: evaluatedSchema.className, |
There was a problem hiding this comment.
There's a potential issue with the prop spreading order. The className property is spread twice:
- First in line 76 as part of
componentProps(sinceclassNameis not filtered out) - Then explicitly in line 78
This means the explicit className on line 78 will always override any className in componentProps, and then the explicit props object can override it again. This creates a confusing precedence:
componentProps.classNameis spread but immediately overriddenevaluatedSchema.classNametakes precedenceevaluatedSchema.props.classNametakes final precedence
Consider either:
- Adding
classNameto the destructuring exclusion list (lines 60-71) so it's not included incomponentProps, OR - Removing line 78 since
classNameis already incomponentPropsand can be overridden byevaluatedSchema.props
The recommended approach is option 1 (add className to exclusions) to maintain explicit control over the prop precedence order.
| // Extract schema metadata properties that should NOT be passed as React props | ||
| const { | ||
| type: _type, | ||
| children: _children, | ||
| body: _body, | ||
| schema: _schema, | ||
| visible: _visible, | ||
| visibleOn: _visibleOn, | ||
| hidden: _hidden, | ||
| hiddenOn: _hiddenOn, | ||
| disabled: _disabled, | ||
| disabledOn: _disabledOn, | ||
| ...componentProps | ||
| } = evaluatedSchema; |
There was a problem hiding this comment.
The properties id, name, label, description, placeholder, data, testId, and ariaLabel are defined in the BaseSchema interface and should potentially be filtered out from the spread, as they are schema metadata rather than component props.
These properties are part of the schema protocol but may not be intended as direct React props for all components. For example:
idis already being passed asdata-obj-idnameis typically for form fields specificallydatais schema metadata, not a component proptestIdshould be transformed todata-testidariaLabelshould be transformed toaria-label
Consider reviewing which BaseSchema properties should be:
- Filtered out entirely (like
data) - Transformed before passing (like
testId→data-testid,ariaLabel→aria-label) - Passed through directly (like
label,placeholderfor input components)
The current implementation may pass unintended props to components that don't expect them, which could cause React warnings about unknown DOM attributes.
| const { | ||
| type: _type, | ||
| children: _children, | ||
| body: _body, | ||
| schema: _schema, | ||
| visible: _visible, | ||
| visibleOn: _visibleOn, | ||
| hidden: _hidden, | ||
| hiddenOn: _hiddenOn, | ||
| disabled: _disabled, | ||
| disabledOn: _disabledOn, | ||
| ...componentProps | ||
| } = evaluatedSchema; | ||
|
|
||
| return React.createElement(Component, { | ||
| schema: evaluatedSchema, | ||
| ...(evaluatedSchema.props || {}), | ||
| ...componentProps, // Spread non-metadata schema properties as props | ||
| ...(evaluatedSchema.props || {}), // Override with explicit props if provided |
There was a problem hiding this comment.
The props property from the schema is not being filtered out in the destructuring (lines 60-71), which means it will be included in componentProps and spread on line 76, and then spread AGAIN on line 77.
This creates duplicate spreading:
...componentProps, // includes evaluatedSchema.props
...(evaluatedSchema.props || {}), // spreads it again
This is redundant and could cause confusion. The props property should be added to the exclusion list alongside other metadata properties:
const {
type: _type,
children: _children,
body: _body,
schema: _schema,
visible: _visible,
visibleOn: _visibleOn,
hidden: _hidden,
hiddenOn: _hiddenOn,
disabled: _disabled,
disabledOn: _disabledOn,
props: _props, // Add this
...componentProps
} = evaluatedSchema;
Then line 77 becomes the single source for spreading the explicit props object.
| type: _type, | ||
| children: _children, | ||
| body: _body, | ||
| schema: _schema, |
There was a problem hiding this comment.
The schema property is being filtered out on line 64, but this might cause issues because the entire schema object is already being passed explicitly as a prop on line 75: schema: evaluatedSchema.
Filtering out schema from componentProps is correct to avoid passing it twice, but the variable name _schema is misleading - it suggests this is the schema being filtered out, when actually it's a property NAMED 'schema' that might exist on the evaluatedSchema object.
For clarity, consider renaming this to indicate it's a property collision being avoided:
schema: _schemaProperty, // Avoid collision with explicit schema prop
This makes it clear that we're filtering out any schema property that might exist on the object to avoid conflicting with the explicit schema prop being passed on line 75.
| schema: _schema, | |
| schema: _schemaProperty, // Avoid collision with explicit `schema` prop below |
Fix Storybook Dashboard Component Blank Display
Problem
The Storybook dashboard component (storybook dashboard 组件) was displaying as blank due to two issues:
childrenarray instead of thewidgetsarray required by theDashboardSchematypeSchemaRenderercomponent in@object-ui/reactonly spreadschema.propsinstead of the entire schema object, preventing components from receiving props likelabelandvalueRoot Cause Analysis
The
DashboardRendererexpects awidgetsarray where each widget has:id: Unique identifiertitle: Optional title for the widgetcomponent: The actual schema to render (e.g., metric, card)The original stories incorrectly passed components directly as
children, causing no widgets to be rendered.Additionally, the
SchemaRendererwas only spreadingschema.props, but many components (likeMetricWidget) expect their props directly on the schema object (e.g.,{ type: 'metric', label: 'Total Revenue', value: '$45,231.89' }).Changes Made
1. Fixed
packages/components/src/stories-json/dashboard.stories.tsxchildrenarray towidgetsarrayidandcomponentproperties2. Fixed
packages/react/src/SchemaRenderer.tsxchildren,body,type, etc. that should NOT be passed as React props (second commit)propsobjectlabel,value)Screenshots
Dashboard - Default Story (3 Metric Cards):
Page Story - Now Working Without Errors:
Testing
Files Changed
packages/components/src/stories-json/dashboard.stories.tsx(schema structure fix)packages/react/src/SchemaRenderer.tsx(prop spreading fix with metadata filtering)Security Summary
No security vulnerabilities discovered or introduced by these changes.
Original prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.