Uh oh!
There was an error while loading. Please reload this page.
Add ViewDataSchema for dual-mode data sourcing in Forms and Tables - #95
Conversation
The latest updates on your projects. Learn more about Vercel for GitHub.
|
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>
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Pull request overview
This pull request adds support for dual-mode data sourcing in Forms and Tables, enabling both protocol-driven (auto-wired to ObjectStack APIs) and custom modes (explicit data sources). The implementation introduces a ViewDataSchema with three provider modes (object, api, value) and integrates it into existing ListView and FormView schemas.
Changes:
- Added
ViewDataSchemawith discriminated union for three data provider modes: protocol-based object binding, custom API endpoints, and static inline data - Added HTTP-related schemas (
HttpMethodSchema,HttpRequestSchema) to support custom API configuration - Integrated optional
dataproperty intoListViewSchemaandFormViewSchemafor backward compatibility
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
| packages/spec/src/ui/view.zod.ts | Core schema definitions for ViewData, HttpRequest, and HttpMethod, plus integration with ListView and FormView |
| packages/spec/src/ui/view.test.ts | Comprehensive test coverage for new schemas including all three provider modes |
| packages/spec/package-lock.json | Version bump from 0.3.0 to 0.3.1 |
Files not reviewed (1)
- packages/spec/package-lock.json: Language not supported
| /** | ||
| * HTTP Method Enum | ||
| */ | ||
| export const HttpMethodSchema = z.enum(['GET', 'POST', 'PUT', 'PATCH', 'DELETE']); |
There was a problem hiding this comment.
This HTTP method enum duplicates the existing HttpMethod schema in packages/spec/src/system/api.zod.ts:6. To maintain consistency and avoid duplication, consider importing and reusing the existing HttpMethod schema instead of defining a new HttpMethodSchema. The existing schema is already exported from the System namespace and uses the same enum values.
| export const ViewDataSchema = z.discriminatedUnion('provider', [ | ||
| z.object({ | ||
| provider: z.literal('object'), | ||
| object: z.string().describe('Target object name'), |
There was a problem hiding this comment.
The object field should include snake_case validation to ensure consistency with the naming convention for machine names throughout the codebase. According to the established pattern (e.g., packages/spec/src/data/dataset.zod.ts:29), object names should be validated with .regex(/^[a-z_][a-z0-9_]*$/) to enforce the snake_case convention for data values.
| url: z.string().describe('API endpoint URL'), | ||
| method: HttpMethodSchema.optional().default('GET').describe('HTTP method'), | ||
| headers: z.record(z.string()).optional().describe('Custom HTTP headers'), | ||
| params: z.record(z.unknown()).optional().describe('Query parameters'), |
There was a problem hiding this comment.
The use of z.unknown() is inconsistent with the rest of the codebase. Throughout this repository, z.any() is used for flexible/dynamic values (as seen in packages/spec/src/api/contract.zod.ts:29, packages/spec/src/data/field.zod.ts:90, and numerous other files). While z.unknown() is technically more type-safe, changing this pattern in isolation creates inconsistency. Consider using z.any() for params to align with established conventions.
| method: HttpMethodSchema.optional().default('GET').describe('HTTP method'), | ||
| headers: z.record(z.string()).optional().describe('Custom HTTP headers'), | ||
| params: z.record(z.unknown()).optional().describe('Query parameters'), | ||
| body: z.unknown().optional().describe('Request body for POST/PUT/PATCH'), |
There was a problem hiding this comment.
The use of z.unknown() is inconsistent with the rest of the codebase. Throughout this repository, z.any() is used for flexible/dynamic values (as seen in packages/spec/src/data/workflow.zod.ts:81, packages/spec/src/api/contract.zod.ts:11, and numerous other files). While z.unknown() is technically more type-safe, changing this pattern in isolation creates inconsistency. Consider using z.any() for body to align with established conventions.
| }), | ||
| z.object({ | ||
| provider: z.literal('value'), | ||
| items: z.array(z.unknown()).describe('Static data array'), |
There was a problem hiding this comment.
The use of z.unknown() is inconsistent with the rest of the codebase. Throughout this repository, z.any() is used for array items with dynamic data (as seen in packages/spec/src/data/dataset.zod.ts:56 and numerous other files). While z.unknown() is technically more type-safe, changing this pattern in isolation creates inconsistency. Consider using z.array(z.any()) for items to align with established conventions.
| z.object({ | ||
| provider: z.literal('api'), | ||
| read: HttpRequestSchema.optional().describe('Configuration for fetching data'), | ||
| write: HttpRequestSchema.optional().describe('Configuration for submitting data (for forms/editable tables)'), | ||
| }), | ||
| z.object({ |
There was a problem hiding this comment.
The api provider allows both read and write to be optional, which means an api provider could be configured without any endpoints. This could lead to runtime issues. Consider adding validation to ensure at least one of read or write is provided when using the api provider, using Zod's .refine() method or making at least the read field required for meaningful API data sources.
| z.object({ | |
| provider: z.literal('api'), | |
| read: HttpRequestSchema.optional().describe('Configuration for fetching data'), | |
| write: HttpRequestSchema.optional().describe('Configuration for submitting data (for forms/editable tables)'), | |
| }), | |
| z.object({ | |
| z | |
| .object({ | |
| provider: z.literal('api'), | |
| read: HttpRequestSchema.optional().describe('Configuration for fetching data'), | |
| write: HttpRequestSchema.optional().describe('Configuration for submitting data (for forms/editable tables)'), | |
| }) | |
| .refine( | |
| (data)=>data.read!==undefined||data.write!==undefined, | |
| { | |
| message: 'api provider requires at least one of "read" or "write"', | |
| } | |
| ), | |
| z.object({ |
| it('should accept api provider with read and write configurations', () => { | ||
| const data: ViewData = { | ||
| provider: 'api', | ||
| read: { | ||
| url: '/api/accounts', | ||
| method: 'GET', | ||
| }, | ||
| write: { | ||
| url: '/api/accounts', | ||
| method: 'POST', | ||
| headers: { 'Content-Type': 'application/json' }, | ||
| }, | ||
| }; | ||
| expect(() => ViewDataSchema.parse(data)).not.toThrow(); | ||
| }); |
There was a problem hiding this comment.
Test coverage is missing for the edge case where an api provider has neither read nor write configured. Consider adding a test case to verify this scenario is handled appropriately (either by validation rejection or documented as acceptable). Additionally, consider testing an api provider with only write configured, which could be a valid use case for write-only forms.
Forms and Tables need to support both protocol-driven (auto-wired to ObjectStack APIs) and custom modes (explicit data sources). This adds a unified
dataconfiguration to enable both patterns without schema duplication.Changes
New Schemas:
ViewDataSchema: Discriminated union onproviderfield with three modes:object: Protocol mode - binds to ObjectStack object by nameapi: Custom API mode - explicit read/write endpointsvalue: Static mode - inline data arrayHttpRequestSchema: API endpoint configuration (url, method, headers, params, body)HttpMethodSchema: HTTP method enumSchema Updates:
ListViewSchema: Added optionaldata: ViewDataSchemaFormViewSchema: Added optionaldata: ViewDataSchemaUsage
Type Safety Notes:
z.unknown()overz.any()for params/body/itemsdataproperty is optional - existing configs unchangedOriginal prompt
This pull request was created from Copilot chat.
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.