Skip to content

Fix dashboard stories schema and SchemaRenderer prop spreading #286

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
hotlong merged 3 commits into from
Jan 30, 2026
Merged

Fix dashboard stories schema and SchemaRenderer prop spreading #286

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
61d3835
Initial plan
Copilot Jan 30, 2026
688e55a
Fix dashboard story schema structure and SchemaRenderer prop spreading
Copilot Jan 30, 2026
7f6148e
Fix React error by excluding schema metadata from prop spreading
Copilot Jan 30, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
66 changes: 38 additions & 28 deletions packages/components/src/stories-json/dashboard.stories.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,24 +25,30 @@ export const Default: Story = {
type: 'dashboard',
columns: 3,
gap: 4,
children: [
widgets: [
{
type: 'metric',
label: 'Total Revenue',
value: '$45,231.89',
className: 'col-span-1'
id: 'metric-1',
component: {
type: 'metric',
label: 'Total Revenue',
value: '$45,231.89',
}
},
{
type: 'metric',
label: 'Active Users',
value: '2,350',
className: 'col-span-1'
id: 'metric-2',
component: {
type: 'metric',
label: 'Active Users',
value: '2,350',
}
},
{
type: 'metric',
label: 'Conversion Rate',
value: '12.5%',
className: 'col-span-1'
id: 'metric-3',
component: {
type: 'metric',
label: 'Conversion Rate',
value: '12.5%',
}
}
]
} as any,
Expand All @@ -54,24 +60,28 @@ export const WithCards: Story = {
type: 'dashboard',
columns: 2,
gap: 6,
children: [
widgets: [
{
type: 'card',
title: 'Schema/Plugins/Sales Overview',
children: [
{ type: 'metric', label: 'Today', value: '$1,234' },
{ type: 'metric', label: 'This Week', value: '$8,456' },
],
className: 'col-span-1'
id: 'card-1',
title: 'Sales Overview',
component: {
type: 'card',
children: [
{ type: 'metric', label: 'Today', value: '$1,234' },
{ type: 'metric', label: 'This Week', value: '$8,456' },
],
}
},
{
type: 'card',
title: 'Schema/Plugins/User Metrics',
children: [
{ type: 'metric', label: 'Online', value: '456' },
{ type: 'metric', label: 'New Today', value: '89' },
],
className: 'col-span-1'
id: 'card-2',
title: 'User Metrics',
component: {
type: 'card',
children: [
{ type: 'metric', label: 'Online', value: '456' },
{ type: 'metric', label: 'New Today', value: '89' },
],
}
}
]
} as any,
Expand Down
19 changes: 18 additions & 1 deletion packages/react/src/SchemaRenderer.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,9 +55,26 @@ export const SchemaRenderer = forwardRef<any, { schema: SchemaNode } & Record<st

// Note: We don't forward the ref to the Component because components in the registry
// may not support refs. The SchemaRenderer itself can still receive refs for its own use.

// Extract schema metadata properties that should NOT be passed as React props
const {
type: _type,
children: _children,
body: _body,
schema: _schema,
Copy link

Copilot AI Jan 30, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
schema: _schema,
schema: _schemaProperty, // Avoid collision with explicit `schema` prop below

Copilot uses AI. Check for mistakes.
visible: _visible,
visibleOn: _visibleOn,
hidden: _hidden,
hiddenOn: _hiddenOn,
disabled: _disabled,
disabledOn: _disabledOn,
...componentProps
} = evaluatedSchema;
Comment on lines +59 to +72
Copy link

Copilot AI Jan 30, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  • id is already being passed as data-obj-id
  • name is typically for form fields specifically
  • data is schema metadata, not a component prop
  • testId should be transformed to data-testid
  • ariaLabel should be transformed to aria-label

Consider reviewing which BaseSchema properties should be:

  1. Filtered out entirely (like data)
  2. Transformed before passing (like testId → data-testid, ariaLabel → aria-label)
  3. Passed through directly (like label, placeholder for 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.

Copilot uses AI. Check for mistakes.

return React.createElement(Component, {
schema: evaluatedSchema,
...(evaluatedSchema.props || {}),
...componentProps, // Spread non-metadata schema properties as props
...(evaluatedSchema.props || {}), // Override with explicit props if provided
Comment on lines +60 to +77
Copy link

Copilot AI Jan 30, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
className: evaluatedSchema.className,
Comment on lines +76 to 78
Copy link

Copilot AI Jan 30, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a potential issue with the prop spreading order. The className property is spread twice:

  1. First in line 76 as part of componentProps (since className is not filtered out)
  2. 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.className is spread but immediately overridden
  • evaluatedSchema.className takes precedence
  • evaluatedSchema.props.className takes final precedence

Consider either:

  1. Adding className to the destructuring exclusion list (lines 60-71) so it's not included in componentProps, OR
  2. Removing line 78 since className is already in componentProps and can be overridden by evaluatedSchema.props

The recommended approach is option 1 (add className to exclusions) to maintain explicit control over the prop precedence order.

Copilot uses AI. Check for mistakes.
'data-obj-id': evaluatedSchema.id,
Comment on lines +60 to 79
Copy link

Copilot AI Jan 30, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. Components might receive both id and data-obj-id props
  2. The id prop might be used as the DOM element's id attribute, which could cause duplicate IDs if multiple instances of the same schema are rendered
  3. It's unclear which id value should take precedence if a component explicitly uses the id prop

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.

Copilot uses AI. Check for mistakes.
'data-obj-type': evaluatedSchema.type,
Expand Down
Loading