Fix component registration conflicts in test environment#316
Fix component registration conflicts in test environment#316hotlong merged 5 commits intocopilot/fix-step-8-errorfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Add manual re-registration of text component to override field widget Co-authored-by: hotlong <50353452+hotlong@users.noreply.github.com>
Co-authored-by: hotlong <50353452+hotlong@users.noreply.github.com>
Co-authored-by: hotlong <50353452+hotlong@users.noreply.github.com>
Move {...props} before explicit props to allow proper overriding
Co-authored-by: hotlong <50353452+hotlong@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR addresses test failures caused by component registration conflicts between @object-ui/fields and @object-ui/components. The primary issue stems from the ComponentRegistry's backward compatibility mechanism, where field widgets registered with namespace 'field' overwrite non-namespaced lookup entries for basic UI components. Additionally, field widgets were missing prop spreads that prevented accessibility attributes from being passed through.
Changes:
- Modified test setup to manually re-register the 'text' component after module imports to restore UI namespace priority
- Added prop spreads (
{...props}) to 14 field widget components to enable proper accessibility attribute propagation - Correctly handled className extraction in widgets requiring class merging (CurrencyField, PasswordField, PercentField)
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| apps/console/vitest.setup.tsx | New test setup file with JSX support and manual text component re-registration |
| apps/console/vitest.setup.ts | Deleted old test setup file (replaced by .tsx version) |
| apps/console/vite.config.ts | Updated setupFiles reference from .ts to .tsx extension |
| packages/fields/src/widgets/TextField.tsx | Added prop spread and removed duplicate spread at end |
| packages/fields/src/widgets/TextAreaField.tsx | Added prop spread to pass accessibility props |
| packages/fields/src/widgets/EmailField.tsx | Added prop spread to pass accessibility props |
| packages/fields/src/widgets/UrlField.tsx | Added prop spread to pass accessibility props |
| packages/fields/src/widgets/PhoneField.tsx | Added prop spread to pass accessibility props |
| packages/fields/src/widgets/DateField.tsx | Added prop spread to pass accessibility props |
| packages/fields/src/widgets/DateTimeField.tsx | Added prop spread to pass accessibility props |
| packages/fields/src/widgets/TimeField.tsx | Added prop spread to pass accessibility props |
| packages/fields/src/widgets/NumberField.tsx | Added prop spread to pass accessibility props |
| packages/fields/src/widgets/PasswordField.tsx | Extracted className, added prop spread |
| packages/fields/src/widgets/PercentField.tsx | Extracted className, added prop spread |
| packages/fields/src/widgets/CurrencyField.tsx | Extracted className, added prop spread, removed duplicate |
| packages/fields/src/widgets/BooleanField.tsx | Added prop spread but incorrect placement causes id override |
| packages/fields/src/widgets/SelectField.tsx | Added prop spread but placed on wrong component |
Comments suppressed due to low confidence (1)
packages/fields/src/widgets/SelectField.tsx:28
- The prop spread is incorrectly placed on the Select root component. Accessibility props like
idandaria-describedbyshould be spread on SelectTrigger, not on the Select root (which is a Radix UI Root component that doesn't accept these props). Move the prop spread from line 23 to line 28 to ensure accessibility attributes are properly applied to the trigger element.
<Select
{...props}
value={value}
onValueChange={onChange}
disabled={readonly}
>
<SelectTrigger className={props.className}>
| // Manually re-register basic text component to override field widget | ||
| // This is necessary because @object-ui/fields has @object-ui/components as a dependency, | ||
| // so components gets loaded BEFORE fields registers its widgets, causing fields to overwrite. |
There was a problem hiding this comment.
The comment explanation is misleading. The actual issue is that when @object-ui/fields registers components with namespace 'field', the Registry's backward compatibility mechanism (lines 97-103 in Registry.ts) also registers them without namespace, overwriting the 'ui' namespace registrations. The fix (re-registering 'text' with namespace 'ui') is correct, but the comment should accurately explain that fields' namespace:'field' registration overwrites the non-namespaced lookup entry, not that "components gets loaded BEFORE fields".
| // Manually re-register basic text component to override field widget | |
| // This is necessary because @object-ui/fields has @object-ui/components as a dependency, | |
| // so components gets loaded BEFORE fields registers its widgets, causing fields to overwrite. | |
| // Manually re-register basic text component to override the field text widget. | |
| // When @object-ui/fields registers its widgets with namespace: 'field', the Registry's | |
| // backward-compatibility logic also registers them without a namespace, overwriting the | |
| // existing non-namespaced 'text' registration from @object-ui/components. We re-register | |
| // 'text' here under the 'ui' namespace so lookups for ui:text resolve to the UI text component. |
| <Checkbox | ||
| {...props} | ||
| id={id} | ||
| checked={!!value} | ||
| onCheckedChange={(checked) => onChange(!!checked)} | ||
| disabled={readonly} | ||
| className={props.className} | ||
| /> |
There was a problem hiding this comment.
The prop spread comes before the explicit id prop, which means any id passed via props will be overwritten by the locally generated id. This defeats the purpose of spreading props for accessibility. Either move {...props} after all explicit props to allow the component to use the generated id as default, or extract id from props and use it as a fallback: const finalId = props.id || config?.name || ....
| <Switch | ||
| {...props} | ||
| id={id} | ||
| checked={!!value} | ||
| onCheckedChange={onChange} | ||
| disabled={readonly} | ||
| className={props.className} | ||
| /> |
There was a problem hiding this comment.
Same issue as the checkbox variant above - the prop spread comes before the explicit id prop, which means any id passed via props will be overwritten by the locally generated id. Either move {...props} after all explicit props, or extract id from props and use it as a fallback.
Test failures occurred because
@object-ui/fieldsdepends on@object-ui/components, causing field widgets to overwrite basic UI components during module initialization. Form fields also lost accessibility attributes due to missing prop spreads.Changes
Test Setup
vitest.setup.ts→.tsxto support manual component re-registrationtextcomponent after all imports to override field widget collisionField Widgets
{...props}spread to 14 widgets to pass throughid,aria-describedby, etc.Example fix:
Test Results
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.