feat: Replace hardcoded cell formatters with @object-ui/fields widgets in object-agrid#391
Open
feat: Replace hardcoded cell formatters with @object-ui/fields widgets in object-agrid#391
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
…ering Co-authored-by: hotlong <50353452+hotlong@users.noreply.github.com>
Co-authored-by: hotlong <50353452+hotlong@users.noreply.github.com>
…lements for fallback renderers Co-authored-by: hotlong <50353452+hotlong@users.noreply.github.com>
Copilot
AI
changed the title
[WIP] Update object-agrid to use defined components for rendering cells
feat: Replace hardcoded cell formatters with @object-ui/fields widgets in object-agrid
Feb 6, 2026
Contributor
There was a problem hiding this comment.
Pull request overview
This pull request refactors the AG Grid plugin to use field widgets from @object-ui/fields instead of hardcoded cell formatters, achieving consistency across the ObjectUI ecosystem and eliminating ~160 lines of duplicated formatting logic.
Changes:
- Introduces a unified rendering approach using field widgets for 21 field types (text, number, boolean, date, currency, email, phone, url, color, rating, image, avatar, etc.)
- Replaces switch-case formatter logic with reusable field widget components that support both read-only and edit modes
- Improves security by ensuring fallback renderers use DOM creation instead of HTML string concatenation
Reviewed changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| packages/plugin-aggrid/src/field-renderers.tsx | New module implementing AG Grid custom renderers/editors using React field widgets with createRoot, including factory functions for field metadata injection |
| packages/plugin-aggrid/src/ObjectAgGridImpl.tsx | Refactored to use field widget renderers instead of hardcoded formatters; removed ~160 lines of switch-case logic and unused imports; fallback renderers now use safe DOM creation |
| packages/plugin-aggrid/package.json | Added @object-ui/fields workspace dependency |
| pnpm-lock.yaml | Updated lockfile with new dependency and platform-specific binary metadata |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
Comment on lines
456
to
466
| case 'lookup': | ||
| case 'master_detail': | ||
| colDef.valueFormatter = (params: any) => { | ||
| if (params.value == null) return ''; | ||
| return Number(params.value).toFixed(precision); | ||
| if (!params.value) return ''; | ||
| // Handle lookup values - could be an object or just an ID | ||
| if (typeof params.value === 'object') { | ||
| return params.value.name || params.value.label || params.value.id || ''; | ||
| } | ||
| return String(params.value); | ||
| }; | ||
| break; |
There was a problem hiding this comment.
The field type 'lookup' appears in both the fieldWidgetTypes array (line 437) and in the fallback switch statement (line 456). This creates redundant code where the lookup type will use the field widget renderer but the fallback case will never be reached. Remove the 'lookup' and 'master_detail' cases from the fallback switch statement since they're already handled by the field widget renderer.
Contributor
Author
There was a problem hiding this comment.
Fixed in 97ae174. Removed the redundant 'lookup' case from the fallback switch since it's already handled by the field widget renderer in line 437.
| ); | ||
| return true; | ||
| } | ||
|
|
There was a problem hiding this comment.
The refresh() method returns false when there's no FieldWidget for the given field type. According to AG Grid's API, returning false means the cell was not refreshed and AG Grid will remove and re-create the cell. For the fallback case (no widget), you should update the textContent and return true to avoid unnecessary DOM manipulation.
Suggested change
| // Fallback to text display when no FieldWidget is available | |
| if (this.eGui) { | |
| this.eGui.textContent = value != null ? String(value) : ''; | |
| return true; | |
| } |
Contributor
Author
There was a problem hiding this comment.
Fixed in 97ae174. The refresh() method now updates textContent for the fallback case and returns true to avoid unnecessary DOM recreation.
Comment on lines
1
to
217
| /** | ||
| * ObjectUI | ||
| * Copyright (c) 2024-present ObjectStack Inc. | ||
| * | ||
| * This source code is licensed under the MIT license found in the | ||
| * LICENSE file in the root directory of this source tree. | ||
| */ | ||
|
|
||
| import React from 'react'; | ||
| import { createRoot, type Root } from 'react-dom/client'; | ||
| import type { ICellRendererParams, ICellEditorParams } from 'ag-grid-community'; | ||
| import type { FieldMetadata } from '@object-ui/types'; | ||
|
|
||
| // Import field widgets | ||
| import { | ||
| TextField, | ||
| NumberField, | ||
| BooleanField, | ||
| SelectField, | ||
| DateField, | ||
| DateTimeField, | ||
| TimeField, | ||
| EmailField, | ||
| PhoneField, | ||
| UrlField, | ||
| CurrencyField, | ||
| PercentField, | ||
| PasswordField, | ||
| TextAreaField, | ||
| ColorField, | ||
| RatingField, | ||
| ImageField, | ||
| AvatarField, | ||
| LookupField, | ||
| SliderField, | ||
| CodeField, | ||
| } from '@object-ui/fields'; | ||
|
|
||
| /** | ||
| * Map field type to field widget component | ||
| */ | ||
| function getFieldWidget(fieldType: string): React.ComponentType<any> | null { | ||
| const widgetMap: Record<string, React.ComponentType<any>> = { | ||
| text: TextField, | ||
| textarea: TextAreaField, | ||
| number: NumberField, | ||
| currency: CurrencyField, | ||
| percent: PercentField, | ||
| boolean: BooleanField, | ||
| select: SelectField, | ||
| date: DateField, | ||
| datetime: DateTimeField, | ||
| time: TimeField, | ||
| email: EmailField, | ||
| phone: PhoneField, | ||
| url: UrlField, | ||
| password: PasswordField, | ||
| color: ColorField, | ||
| rating: RatingField, | ||
| image: ImageField, | ||
| avatar: AvatarField, | ||
| lookup: LookupField, | ||
| slider: SliderField, | ||
| code: CodeField, | ||
| }; | ||
|
|
||
| return widgetMap[fieldType] || null; | ||
| } | ||
|
|
||
| /** | ||
| * AG Grid Cell Renderer using Field Widgets (Read-only mode) | ||
| */ | ||
| export class FieldWidgetCellRenderer { | ||
| public eGui!: HTMLDivElement; | ||
| public root: Root | null = null; | ||
|
|
||
| init(params: ICellRendererParams & { field: FieldMetadata }) { | ||
| const { value, field } = params; | ||
| const FieldWidget = getFieldWidget(field.type); | ||
|
|
||
| this.eGui = document.createElement('div'); | ||
| this.eGui.className = 'field-widget-cell'; | ||
|
|
||
| if (FieldWidget) { | ||
| this.root = createRoot(this.eGui); | ||
| this.root.render( | ||
| <FieldWidget | ||
| value={value} | ||
| onChange={() => {}} // No-op for read-only mode | ||
| field={field} | ||
| readonly={true} | ||
| /> | ||
| ); | ||
| } else { | ||
| // Fallback to text display | ||
| this.eGui.textContent = value != null ? String(value) : ''; | ||
| } | ||
| } | ||
|
|
||
| getGui() { | ||
| return this.eGui; | ||
| } | ||
|
|
||
| refresh(params: ICellRendererParams & { field: FieldMetadata }): boolean { | ||
| const { value, field } = params; | ||
| const FieldWidget = getFieldWidget(field.type); | ||
|
|
||
| if (FieldWidget && this.root) { | ||
| this.root.render( | ||
| <FieldWidget | ||
| value={value} | ||
| onChange={() => {}} // No-op for read-only mode | ||
| field={field} | ||
| readonly={true} | ||
| /> | ||
| ); | ||
| return true; | ||
| } | ||
|
|
||
| return false; | ||
| } | ||
|
|
||
| destroy() { | ||
| if (this.root) { | ||
| this.root.unmount(); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * AG Grid Cell Editor using Field Widgets (Edit mode) | ||
| */ | ||
| export class FieldWidgetCellEditor { | ||
| public eGui!: HTMLDivElement; | ||
| public root: Root | null = null; | ||
| public currentValue: unknown; | ||
| public params!: ICellEditorParams & { field: FieldMetadata }; | ||
|
|
||
| init(params: ICellEditorParams & { field: FieldMetadata }) { | ||
| this.params = params; | ||
| this.currentValue = params.value; | ||
| const { field } = params; | ||
| const FieldWidget = getFieldWidget(field.type); | ||
|
|
||
| this.eGui = document.createElement('div'); | ||
| this.eGui.className = 'field-widget-editor'; | ||
|
|
||
| if (FieldWidget) { | ||
| this.root = createRoot(this.eGui); | ||
| this.root.render( | ||
| <FieldWidget | ||
| value={this.currentValue} | ||
| onChange={(newValue: any) => { | ||
| this.currentValue = newValue; | ||
| }} | ||
| field={field} | ||
| readonly={false} | ||
| /> | ||
| ); | ||
| } else { | ||
| // Fallback to input element | ||
| const input = document.createElement('input'); | ||
| input.value = this.currentValue != null ? String(this.currentValue) : ''; | ||
| input.className = 'ag-input-field-input ag-text-field-input'; | ||
| input.addEventListener('input', (e) => { | ||
| this.currentValue = (e.target as HTMLInputElement).value; | ||
| }); | ||
| this.eGui.appendChild(input); | ||
| setTimeout(() => input.focus(), 0); | ||
| } | ||
| } | ||
|
|
||
| getGui() { | ||
| return this.eGui; | ||
| } | ||
|
|
||
| getValue() { | ||
| return this.currentValue; | ||
| } | ||
|
|
||
| destroy() { | ||
| if (this.root) { | ||
| this.root.unmount(); | ||
| } | ||
| } | ||
|
|
||
| isPopup(): boolean { | ||
| // Return true for complex widgets that need more space | ||
| const popupTypes = ['date', 'datetime', 'select', 'lookup', 'color']; | ||
| return popupTypes.includes(this.params.field.type); | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Factory function to create cell renderer with field metadata | ||
| */ | ||
| export function createFieldCellRenderer(field: FieldMetadata) { | ||
| return class extends FieldWidgetCellRenderer { | ||
| init(params: ICellRendererParams) { | ||
| super.init({ ...params, field }); | ||
| } | ||
| refresh(params: ICellRendererParams): boolean { | ||
| return super.refresh({ ...params, field }); | ||
| } | ||
| }; | ||
| } | ||
|
|
||
| /** | ||
| * Factory function to create cell editor with field metadata | ||
| */ | ||
| export function createFieldCellEditor(field: FieldMetadata) { | ||
| return class extends FieldWidgetCellEditor { | ||
| init(params: ICellEditorParams) { | ||
| super.init({ ...params, field }); | ||
| } | ||
| }; | ||
| } |
There was a problem hiding this comment.
The new field-renderers.tsx module introduces critical rendering and editing logic for AG Grid cells but lacks test coverage. Given that this package has existing test coverage (see index.test.ts and object-aggrid.test.ts), tests should be added for the new field renderer classes. Consider adding tests for: 1) Widget mapping in getFieldWidget(), 2) Renderer lifecycle (init, refresh, destroy), 3) Editor lifecycle and value handling, 4) Fallback behavior when widgets don't exist, and 5) Memory cleanup (root.unmount()).
Contributor
Author
There was a problem hiding this comment.
Fixed in 97ae174. Added comprehensive test coverage with 17 new tests in field-renderers.test.tsx covering widget mapping, renderer/editor lifecycle, fallback behavior, and memory cleanup. All 29 tests now passing.
Comment on lines
42
to
66
| function getFieldWidget(fieldType: string): React.ComponentType<any> | null { | ||
| const widgetMap: Record<string, React.ComponentType<any>> = { | ||
| text: TextField, | ||
| textarea: TextAreaField, | ||
| number: NumberField, | ||
| currency: CurrencyField, | ||
| percent: PercentField, | ||
| boolean: BooleanField, | ||
| select: SelectField, | ||
| date: DateField, | ||
| datetime: DateTimeField, | ||
| time: TimeField, | ||
| email: EmailField, | ||
| phone: PhoneField, | ||
| url: UrlField, | ||
| password: PasswordField, | ||
| color: ColorField, | ||
| rating: RatingField, | ||
| image: ImageField, | ||
| avatar: AvatarField, | ||
| lookup: LookupField, | ||
| slider: SliderField, | ||
| code: CodeField, | ||
| }; | ||
|
|
There was a problem hiding this comment.
The getFieldWidget() function recreates the widgetMap object on every call, which is inefficient. The widgetMap should be defined as a module-level constant outside the function to avoid unnecessary object allocations. This will improve performance, especially when rendering large grids with many cells.
Suggested change
| function getFieldWidget(fieldType: string): React.ComponentType<any> | null { | |
| const widgetMap: Record<string, React.ComponentType<any>> = { | |
| text: TextField, | |
| textarea: TextAreaField, | |
| number: NumberField, | |
| currency: CurrencyField, | |
| percent: PercentField, | |
| boolean: BooleanField, | |
| select: SelectField, | |
| date: DateField, | |
| datetime: DateTimeField, | |
| time: TimeField, | |
| email: EmailField, | |
| phone: PhoneField, | |
| url: UrlField, | |
| password: PasswordField, | |
| color: ColorField, | |
| rating: RatingField, | |
| image: ImageField, | |
| avatar: AvatarField, | |
| lookup: LookupField, | |
| slider: SliderField, | |
| code: CodeField, | |
| }; | |
| const widgetMap: Record<string, React.ComponentType<any>> = { | |
| text: TextField, | |
| textarea: TextAreaField, | |
| number: NumberField, | |
| currency: CurrencyField, | |
| percent: PercentField, | |
| boolean: BooleanField, | |
| select: SelectField, | |
| date: DateField, | |
| datetime: DateTimeField, | |
| time: TimeField, | |
| email: EmailField, | |
| phone: PhoneField, | |
| url: UrlField, | |
| password: PasswordField, | |
| color: ColorField, | |
| rating: RatingField, | |
| image: ImageField, | |
| avatar: AvatarField, | |
| lookup: LookupField, | |
| slider: SliderField, | |
| code: CodeField, | |
| }; | |
| function getFieldWidget(fieldType: string): React.ComponentType<any> | null { |
Contributor
Author
There was a problem hiding this comment.
Fixed in 97ae174. Moved widgetMap to module level to avoid object recreation on every call, improving performance especially for large grids.
📦 Bundle Size Report
Size Limits
|
Contributor
|
@copilot fix ci build and test |
…resh logic, and add comprehensive test coverage Co-authored-by: hotlong <50353452+hotlong@users.noreply.github.com>
Contributor
Author
CI build and test issues have been fixed in commit 97ae174. All checks now passing:
The changes address all code review feedback including performance optimization, improved refresh logic, removal of redundant code, and comprehensive test coverage. |
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
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.
✅ Implementation Complete: Integrate packages/fields into object-agrid
Objective
Improve object-agrid to use field widget components from packages/fields for rendering cells in both read-only and edit modes.
✅ All Changes Completed
Roottype from react-dom/client instead ofanyunknowntype for currentValue instead ofanyTechnical Implementation
Added Files:
packages/plugin-aggrid/src/field-renderers.tsx(221 lines)widgetMap: Module-level constant mapping 21 field types to widgets (performance optimization)getFieldWidget(): Returns widget component for a given field typeFieldWidgetCellRenderer: AG Grid cell renderer using field widgets in readonly modeFieldWidgetCellEditor: AG Grid cell editor using field widgets for editingRoottype from react-dom/clientpackages/plugin-aggrid/src/field-renderers.test.tsx(328 lines)Modified Files:
packages/plugin-aggrid/package.json: Added@object-ui/fieldsworkspace dependencypackages/plugin-aggrid/src/ObjectAgGridImpl.tsx:Field Types Supported
✅ 21 field types with full read-only and edit mode support:
Code Quality Metrics
✅ Type Safety: Explicit types, no
anyin new code✅ Security: CodeQL scan passed (0 vulnerabilities)
✅ Testing: 29/29 tests passing (including 17 new tests for field-renderers)
✅ Test Coverage: Comprehensive coverage of renderer/editor lifecycle, fallback, and edge cases
✅ Build: Clean build with no errors
✅ Performance: Module-level widgetMap prevents object recreation on every call
✅ Maintainability: -105 lines of code (replaced hardcoded logic with reusable components)
Performance Improvements
✅ widgetMap defined at module level (avoids recreation on every getFieldWidget call)
✅ Improved refresh() logic returns true to avoid unnecessary DOM manipulation
Test Coverage Summary
New test file:
field-renderers.test.tsxBenefits Achieved
✅ Consistency: All field types now render using the same widgets across forms, grids, and other components
✅ Maintainability: Centralized field rendering logic in
@object-ui/fieldspackage✅ Extensibility: Easy to add new field types - just update the widget map
✅ Security: DOM-based rendering prevents XSS vulnerabilities
✅ Performance: Optimized widget lookup with module-level constant
✅ Quality: Comprehensive test coverage ensures reliability
✅ DX: Type-safe implementation with IntelliSense support
✅ Edit Support: Automatic edit mode for all 21 supported field types
Original prompt
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.