Conversation
…tion 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>
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>
|
类似的,开发ObjectForm |
|
Co-authored-by: hotlong <50353452+hotlong@users.noreply.github.com>
✅ 已完成 ObjectForm 组件开发 (ObjectForm component completed) Created a metadata-driven form component similar to ObjectGridTable: Core Features:
Usage: import { ObjectForm } from '@objectos/ui';
<ObjectForm
objectConfig={objectConfig}
onSubmit={handleSubmit}
initialValues={data} // for editing
/>Documentation:
Commit: 7a924e1 |
Co-authored-by: hotlong <50353452+hotlong@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This pull request adds metadata-driven UI components to ObjectOS, enabling automatic generation of AG Grid tables and forms from ObjectQL metadata. The implementation eliminates the need for manual column and field configuration by recognizing field types and applying appropriate renderers and validation.
Changes:
- Added
ObjectGridTablecomponent with automatic column generation and type-specific cell renderers for all ObjectQL field types - Added
ObjectFormcomponent with automatic field generation and validation based on metadata configuration - Integrated
@objectql/typesdependency for type consistency across the platform
Reviewed changes
Copilot reviewed 14 out of 15 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
packages/ui/src/components/object-grid-table.tsx |
Core AG Grid component with 8 specialized cell renderers for boolean, date, number, currency, percent, select, lookup, email, and URL field types |
packages/ui/src/components/object-form.tsx |
Metadata-driven form component with React Hook Form integration and automatic validation rule generation |
packages/ui/src/components/examples/ObjectGridTableExample.tsx |
Demo component showcasing all field type renderers with sample data |
packages/ui/src/components/examples/ObjectFormExample.tsx |
Demo component demonstrating form creation and editing modes |
apps/web/src/components/dashboard/EnhancedObjectListView.tsx |
Production example integrating ObjectGridTable with API data fetching |
packages/ui/src/index.ts |
Export statements for new components and types |
packages/ui/package.json |
Added @objectql/types dependency |
packages/ui/OBJECT_GRID_TABLE.md |
Comprehensive English documentation for ObjectGridTable |
packages/ui/OBJECT_GRID_TABLE_CN.md |
Chinese documentation for ObjectGridTable |
packages/ui/OBJECT_FORM.md |
Comprehensive English documentation for ObjectForm |
packages/ui/OBJECT_FORM_CN.md |
Chinese documentation for ObjectForm |
docs/OBJECT_GRID_INTEGRATION.md |
Integration guide for adopting ObjectGridTable |
IMPLEMENTATION_SUMMARY.md |
Technical implementation summary |
OBJECTFORM_IMPLEMENTATION.md |
ObjectForm implementation summary |
| const getHeaders = () => { | ||
| const headers: Record<string, string> = { 'Content-Type': 'application/json' }; | ||
| // Use the actual user ID from props instead of hard-coded value | ||
| if (user?.id || user?._id) { | ||
| headers['x-user-id'] = user.id || user._id; | ||
| } | ||
| return headers; | ||
| }; |
There was a problem hiding this comment.
The getHeaders function is recreated on every render, and is used as a dependency in useEffect hooks. This can cause the useEffect hooks to run more often than necessary because the function reference changes on each render. Move getHeaders outside the component or wrap it in useCallback to prevent unnecessary re-fetches of metadata and data.
There was a problem hiding this comment.
Fixed all code review issues in commit 298ad6e:
- ✅
getHeadersnow wrapped inuseCallbackto prevent unnecessary re-renders - ✅ Dependencies updated to use
getHeadersinstead ofuser - ✅ Added XSS prevention with email/URL sanitization
- ✅ Added date validation with
isNaNcheck - ✅ Improved type safety (RegisterOptions, better object checks)
All changes tested and building successfully.
| const EmailCellRenderer = (props: ICellRendererParams) => { | ||
| const { value } = props | ||
|
|
||
| if (!value) { | ||
| return <span className="text-muted-foreground">-</span> | ||
| } | ||
|
|
||
| return ( | ||
| <a | ||
| href={`mailto:${value}`} | ||
| className="text-primary hover:underline" | ||
| onClick={(e) => e.stopPropagation()} | ||
| > | ||
| {value} | ||
| </a> | ||
| ) | ||
| } | ||
|
|
||
| /** | ||
| * Cell renderer for URL fields | ||
| */ | ||
| const UrlCellRenderer = (props: ICellRendererParams) => { | ||
| const { value } = props | ||
|
|
||
| if (!value) { | ||
| return <span className="text-muted-foreground">-</span> | ||
| } | ||
|
|
||
| return ( | ||
| <a | ||
| href={value} | ||
| target="_blank" | ||
| rel="noopener noreferrer" | ||
| className="text-primary hover:underline" | ||
| onClick={(e) => e.stopPropagation()} | ||
| > | ||
| {value} | ||
| </a> | ||
| ) |
There was a problem hiding this comment.
The email and URL renderers create links without sanitizing the input values. For email fields, a malicious value could potentially inject JavaScript through the mailto: protocol (e.g., javascript:alert('XSS')). For URL fields, while the code requires http:// or https://, there's no validation of the actual URL content before rendering it as a link. Consider validating/sanitizing these values before using them in href attributes to prevent potential XSS attacks.
There was a problem hiding this comment.
Fixed in commit 298ad6e with sanitization functions:
sanitizeEmail(): Blocks javascript: protocol injectionsanitizeUrl(): Only allows http/https, blocks dangerous protocols
| try { | ||
| const date = value instanceof Date ? value : new Date(value) | ||
| // Format based on field type from colDef | ||
| const extendedColDef = colDef as ExtendedColDef | ||
| const fieldType = extendedColDef.fieldType | ||
|
|
||
| if (fieldType === 'datetime') { | ||
| return ( | ||
| <div className="flex items-center gap-1.5"> | ||
| <CalendarIcon className="w-3 h-3 text-muted-foreground" /> | ||
| <span>{format(date, "PPp")}</span> | ||
| </div> | ||
| ) | ||
| } | ||
|
|
||
| return ( | ||
| <div className="flex items-center gap-1.5"> | ||
| <CalendarIcon className="w-3 h-3 text-muted-foreground" /> | ||
| <span>{format(date, "PP")}</span> | ||
| </div> | ||
| ) | ||
| } catch (e) { | ||
| return <span className="text-muted-foreground">{String(value)}</span> | ||
| } | ||
| } |
There was a problem hiding this comment.
The DateCellRenderer does not handle invalid dates gracefully. If the date parsing fails (caught in the try-catch), it displays the raw value as a string. However, there's no validation to check if the Date object is actually valid after construction. JavaScript's Date constructor can create "Invalid Date" objects that won't throw an error but will cause the format function to throw. Consider checking isNaN(date.getTime()) after creating the Date object to ensure it's valid before calling format.
There was a problem hiding this comment.
Fixed in commit 298ad6e - added isNaN(date.getTime()) check before formatting to handle Invalid Date objects gracefully.
| * Generate validation rules for react-hook-form based on field config | ||
| */ | ||
| function getValidationRules(fieldConfig: FieldConfig) { | ||
| const rules: any = {} |
There was a problem hiding this comment.
The validation rules object uses any type instead of a more specific type from react-hook-form. This reduces type safety and could lead to runtime errors if incorrect validation rules are constructed. Consider using the proper type from react-hook-form, such as RegisterOptions or a custom interface that extends it, to ensure type safety for the validation rules.
There was a problem hiding this comment.
Fixed in commit 298ad6e - changed type from any to RegisterOptions from react-hook-form for better type safety.
| value: /^[A-Z0-9._%+-]+@[A-Z0-9.-]+\.[A-Z]{2,}$/i, | ||
| message: 'Invalid email address' | ||
| } |
There was a problem hiding this comment.
The email validation regex may not handle all valid email formats correctly. The pattern /^[A-Z0-9._%+-]+@[A-Z0-9.-]+\.[A-Z]{2,}$/i does not allow for top-level domains longer than the minimum of 2 characters, but some newer TLDs can be longer. Additionally, it doesn't handle special cases like multiple consecutive dots or trailing dots in the domain, which could lead to false positives. Consider using a more robust email validation pattern or a library like validator.js for more comprehensive email validation.
There was a problem hiding this comment.
Fixed in commit 3ae142d - improved email regex to handle longer TLDs and better domain validation:
/^[a-zA-Z0-9.!#$%&'*+/=?^_{|}~-]+@a-zA-Z0-9?(?:.a-zA-Z0-9?)*$/`
| if (fieldConfig.regex) { | ||
| rules.pattern = { | ||
| value: new RegExp(fieldConfig.regex), | ||
| message: 'Invalid format' | ||
| } | ||
| } | ||
|
|
||
| // Email validation | ||
| if (fieldConfig.type === 'email') { | ||
| rules.pattern = { | ||
| value: /^[A-Z0-9._%+-]+@[A-Z0-9.-]+\.[A-Z]{2,}$/i, | ||
| message: 'Invalid email address' | ||
| } | ||
| } | ||
|
|
||
| // URL validation | ||
| if (fieldConfig.type === 'url') { | ||
| rules.pattern = { | ||
| value: /^https?:\/\/.+/i, | ||
| message: 'Invalid URL (must start with http:// or https://)' | ||
| } | ||
| } |
There was a problem hiding this comment.
When a field has type 'email' or 'url' AND a custom regex pattern is defined, the custom regex will be overwritten by the built-in email/URL pattern. The code should check if a custom regex already exists and either combine the validations or give precedence to the custom regex. This could break user expectations if they define a custom regex for stricter validation on email or URL fields.
There was a problem hiding this comment.
Fixed in commit 3ae142d - custom regex now takes precedence. Built-in email/URL patterns only apply when !fieldConfig.regex is true.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: hotlong <50353452+hotlong@users.noreply.github.com>
Co-authored-by: hotlong <50353452+hotlong@users.noreply.github.com>
Deleted OBJECT_FORM_CN.md and OBJECT_GRID_TABLE_CN.md documentation files. Moved example files to a new location. Added @objectql/types dependency to apps/web/package.json.
Enhanced the authentication client to support PostgreSQL, MongoDB, and SQLite databases. Updated configuration to use a file-based SQLite database by default. Added 'better-sqlite3' and 'mongodb' dependencies, and improved user role assignment logic for all supported databases.
…development workflow
…ridTable integration
Code Review Fixes - Final Update ✅
object-form.tsx
✅ Fixed custom regex precedence issue (Comment #2680739153)
!fieldConfig.regex✅ Improved email validation regex (Comment #2680739146)
/^[a-zA-Z0-9.!#$%&'*+/=?^_{|}~-]+@a-zA-Z0-9?(?:.a-zA-Z0-9?)*$/`Previously Fixed (in commit 298ad6e)
All code review issues now fully addressed. Build passes ✅
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.