refactor: Switch to project definition creator pattern for project definitions#553
Conversation
…to use it - Also move settings to settings.general
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
ENG-709 Switch project definition schema to use createSchema pattern
Allows fewer hacks of the Zod validator to pass around plugins and make for smoother v4 upgrade experience |
|
WalkthroughThis update restructures the project definition schema, moving general configuration fields (such as name, package scope, and port offset) from the root level into a nested Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant WebUI
participant Server
participant Schema
participant Migration
User->>WebUI: Submit project settings form
WebUI->>Server: Send settings as { settings: { general: { ... } } }
Server->>Schema: Validate with createProjectDefinitionSchema
Schema-->>Server: Validated/nested config
Server->>Migration: Apply migration013MoveGeneralSettings (if needed)
Migration-->>Server: Migrated config
Server-->>WebUI: Respond with updated project definition
WebUI-->>User: Display updated project info
sequenceDiagram
participant Server
participant FileSystem
participant ZodSchema
Server->>FileSystem: Read project definition JSON
FileSystem-->>Server: Return JSON string
Server->>ZodSchema: Parse with simpleProjectDefinitionSchema
ZodSchema-->>Server: Parsed object (supports both old and new formats)
Server-->>Server: Extract project name from settings.general or root
Server-->>Client: Return project info
sequenceDiagram
participant Migration
participant OldConfig
participant NewConfig
OldConfig->>Migration: Provide flat config with root-level fields
Migration->>NewConfig: Move fields into settings.general
NewConfig-->>Migration: Return nested config
📜 Recent review detailsConfiguration used: .coderabbit.yaml ⛔ Files ignored due to path filters (5)
📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (1)
🔇 Additional comments (2)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
kingston
changed the title
kingston
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (16)
CLAUDE.md (9)
186-188: Define the “MCP” acronym and ensure header consistency.The new section header introduces “MCP” without first defining the acronym. For clarity, expand the acronym on first use (e.g., “Metadata Control Plane (MCP)”) or link to its definition. Also capitalize “Docs” consistently in the header and verify there’s a blank line above and below the header to conform with existing markdown style.
189-193: Refine introductory sentence for clarity and grammar.The sentence is a bit verbose and “up to date” should be hyphenated. Consider tightening the language.
- Please interact with the baseplate-docs MCP to get the most up to date information on the project. Here are the following tips: + Use the Baseplate Docs MCP to retrieve the most up-to-date project documentation. Here are some tips:
193-196: Use inline code formatting for identifiers and commands.GUIDs and command names are easier to spot when wrapped in backticks. For example:
- - Creating Design Docs: Search for the document by id "e84a9e72-9980-41f6-bdda-296736c5ff69" for the design doc template + - Creating design docs: Search for the document by ID `e84a9e72-9980-41f6-bdda-296736c5ff69` for the design doc template
197-200: Capitalize “Keys” in the subheading for consistency.All other subheadings use title case. Change “Collection keys:” → “Collection Keys:”.
- Collection keys: + Collection Keys:
202-211: Format commands and URIs as inline code.Make commands (
get-document,list-documents) and URIs stand out by wrapping them in backticks:- - get-document - Retrieve full document content by URI (format: - documents://collection-name/path/to/doc.md) + - `get-document` – Retrieve full document content by URI (format: + `documents://collection-name/path/to/doc.md`)Also prefer an en-dash or em-dash for list separators.
212-218: Consistently inline-code all tool names.Similarly, wrap search commands to improve readability:
- - search-documents - Semantic search returning full documents with - similarity scores + - `search-documents` – Semantic search returning full documents with similarity scores
219-222: Inline-code the single command in this section.- - list-collections - List all available collections + - `list-collections` – List all available collections
223-228: Clarify prerequisites for document editing commands.The note “(if enabled)” is vague. Consider adding a brief prerequisite or linking to permission docs:
- `edit-document` – Replace entire document content (requires Editor role) - `inline-edit` – Make specific edits within a document (requires Editor role) - `create-document` – Create new document in a collection (requires Contributor role)
229-237: Use fenced code blocks for usage examples.Long commands with arguments are clearer in code fences:
- - Find specific info: search-rag-documents with query "how to configure authentication" + ```shell + search-rag-documents --query "how to configure authentication" + ```Repeat for other examples to improve scannability.
packages/project-builder-lib/src/schema/creator/schema-creator.ts (1)
8-12: Consider simplifying or documenting the purpose of this wrapper.The
definitionSchemafunction appears to be a simple pass-through wrapper. Consider either:
- Documenting why this indirection is necessary
- Simplifying if it's just a pass-through
Current implementation:
-export function definitionSchema<T extends z.ZodTypeAny>( - creator: DefinitionSchemaCreator<T>, -): (options: DefinitionSchemaCreatorOptions) => T { - return (options) => creator(options); -}Could be simplified to just using the creator directly, unless there's a specific reason for this wrapper pattern.
packages/utils/src/validators/transform-with-dynamic-schema.ts (3)
10-14: Value-path is limited to first-level keys – consider supporting deep paths
valuePathis typed askeyof TData, so the helper can only target top-level properties.
If callers ever need to validatesettings.general.name(two levels deep) they will have to add a second wrapper.
Exposing a dot-separated string or accepting an array of path segments would make the helper more generally useful.
25-33:z.NEVERis returned on failure – clarify intent in the JSDoc
Returningz.NEVERis the correct Zod escape-hatch, but it is not obvious to unfamiliar readers why the return typeTDatasuddenly becomesnever.
A single sentence in the doc-block explaining that “z.NEVERaborts the transform and forwards issues” will avoid future confusion.
28-30: Path construction may drop numeric index information
issue.pathcan already contain path segments; pre-pending[valuePath]is fine, but castingvaluePath as stringloses the ability to keep numeric keys (e.g. arrays) type-safe.
Consider:- path: [...(valuePath ? [valuePath as string] : []), ...issue.path], + path: [...(valuePath ? [valuePath] : []), ...issue.path],
keyof TDataalready includesstring | number | symbol, which matches Zod’s path segment type.packages/project-builder-lib/src/schema/settings.ts (2)
22-31: Port-offset validation accepts floats before.int()– swap the order for clearer error messages
When a caller supplies1234.5, the current chain triggers the generic “must be an integer” message after the custom multiple-of-1000 check.
Placing.int()first guarantees users see the more specific refinement only after the value is confirmed to be an integer:- portOffset: z - .number() - .min(1000) - .max(60_000) - .int() + portOffset: z + .number() + .int() + .min(1000) + .max(60_000)
37-43: Unused context parameter – keeps type-safety but may violatestrictFunctionTypes
definitionSchema(() => z.object({...}))ignores the incomingctx.
UnderstrictFunctionTypesthis assignment is not type-safe because the expected signature is(ctx) => T.
Safer pattern:-export const createSettingsSchema = definitionSchema(() => +export const createSettingsSchema = definitionSchema((_ctx) =>or accept the context and document why it is unused.
packages/project-builder-lib/src/schema/creator/types.ts (1)
5-12:OptionsandContextinterfaces are identical – consolidate to one type
BothDefinitionSchemaCreatorOptionsandDefinitionSchemaParserContextonly carryplugins.
Maintaining two aliases invites accidental divergence. Consider:-export interface DefinitionSchemaCreatorOptions { - plugins: PluginImplementationStore; -} - -export interface DefinitionSchemaParserContext { +export interface DefinitionSchemaContext { plugins: PluginImplementationStore; }and reuse
DefinitionSchemaContexteverywhere.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Lite
📒 Files selected for processing (35)
CLAUDE.md(1 hunks)packages/project-builder-cli/tests/fixtures/server-fixture.test-helper.ts(1 hunks)packages/project-builder-cli/tests/settings.spec.ts(1 hunks)packages/project-builder-lib/src/definition/plugins/plugin-utils.ts(2 hunks)packages/project-builder-lib/src/definition/project-definition-container.test-helper.ts(3 hunks)packages/project-builder-lib/src/definition/project-definition-container.ts(2 hunks)packages/project-builder-lib/src/migrations/index.ts(2 hunks)packages/project-builder-lib/src/migrations/migration-013-move-general-settings.ts(1 hunks)packages/project-builder-lib/src/parser/parser.ts(3 hunks)packages/project-builder-lib/src/plugins/spec/web-config-spec.ts(1 hunks)packages/project-builder-lib/src/references/types.ts(1 hunks)packages/project-builder-lib/src/schema/creator/index.ts(1 hunks)packages/project-builder-lib/src/schema/creator/schema-creator.ts(1 hunks)packages/project-builder-lib/src/schema/creator/types.ts(1 hunks)packages/project-builder-lib/src/schema/index.ts(1 hunks)packages/project-builder-lib/src/schema/project-definition.ts(2 hunks)packages/project-builder-lib/src/schema/settings.ts(1 hunks)packages/project-builder-server/src/api/projects.ts(1 hunks)packages/project-builder-server/src/compiler/admin/index.ts(4 hunks)packages/project-builder-server/src/compiler/app-entry-builder.ts(1 hunks)packages/project-builder-server/src/compiler/backend/fastify.ts(1 hunks)packages/project-builder-server/src/compiler/backend/index.ts(1 hunks)packages/project-builder-server/src/compiler/backend/utils.ts(2 hunks)packages/project-builder-server/src/compiler/web/index.ts(2 hunks)packages/project-builder-server/src/service/builder-service.ts(1 hunks)packages/project-builder-server/src/sync/build-project.ts(1 hunks)packages/project-builder-web/src/app/AppLayout/AppBreadcrumbs.tsx(3 hunks)packages/project-builder-web/src/app/ProjectDefinitionProvider/NewProjectCard.tsx(3 hunks)packages/project-builder-web/src/pages/apps/edit.page.tsx(1 hunks)packages/project-builder-web/src/pages/settings/project-settings.tsx(2 hunks)packages/project-builder-web/src/pages/settings/template-extractor.tsx(2 hunks)packages/project-builder-web/src/pages/settings/theme-builder.tsx(2 hunks)packages/utils/src/validators/index.ts(1 hunks)packages/utils/src/validators/transform-with-dynamic-schema.ts(1 hunks)packages/utils/src/validators/transform-with-dynamic-schema.unit.test.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (15)
packages/project-builder-server/src/sync/build-project.ts (1)
packages/project-builder-server/src/sync/generate-for-directory.ts (1)
generateForDirectory(122-259)
packages/project-builder-server/src/compiler/backend/index.ts (1)
packages/core-generators/src/generators/node/_composers/node.ts (1)
composeNodeGenerator(13-26)
packages/project-builder-server/src/compiler/backend/fastify.ts (1)
packages/fastify-generators/src/generators/core/readme/readme.generator.ts (1)
readmeGenerator(15-46)
packages/project-builder-lib/src/migrations/index.ts (1)
packages/project-builder-lib/src/migrations/migration-013-move-general-settings.ts (1)
migration013MoveGeneralSettings(40-70)
packages/project-builder-server/src/compiler/admin/index.ts (2)
packages/react-generators/src/generators/core/_composers/react.composer.ts (1)
composeReactGenerators(17-36)packages/core-generators/src/generators/node/_composers/node.ts (1)
composeNodeGenerator(13-26)
packages/project-builder-lib/src/schema/creator/schema-creator.ts (1)
packages/project-builder-lib/src/schema/creator/types.ts (2)
DefinitionSchemaCreator(13-15)DefinitionSchemaCreatorOptions(5-7)
packages/project-builder-lib/src/definition/project-definition-container.ts (3)
packages/project-builder-lib/src/references/fix-ref-deletions.ts (1)
FixRefDeletionResult(30-32)packages/project-builder-lib/src/schema/creator/types.ts (1)
InferDefinitionSchema(17-19)packages/project-builder-lib/src/schema/project-definition.ts (1)
createProjectDefinitionSchema(38-50)
packages/project-builder-server/src/compiler/web/index.ts (1)
packages/core-generators/src/generators/node/_composers/node.ts (1)
composeNodeGenerator(13-26)
packages/project-builder-lib/src/definition/plugins/plugin-utils.ts (1)
packages/project-builder-lib/src/schema/project-definition.ts (1)
ProjectDefinition(56-58)
packages/project-builder-lib/src/migrations/migration-013-move-general-settings.ts (1)
packages/project-builder-lib/src/migrations/types.ts (1)
createSchemaMigration(30-34)
packages/project-builder-lib/src/definition/project-definition-container.test-helper.ts (1)
packages/project-builder-lib/src/schema/project-definition.ts (1)
createProjectDefinitionSchema(38-50)
packages/project-builder-web/src/app/ProjectDefinitionProvider/NewProjectCard.tsx (1)
packages/project-builder-lib/src/schema/settings.ts (2)
GeneralSettingsInput(33-33)generalSettingsSchema(8-31)
packages/project-builder-lib/src/schema/creator/types.ts (1)
packages/project-builder-lib/src/plugins/schema/store.ts (1)
PluginImplementationStore(3-25)
packages/project-builder-lib/src/schema/settings.ts (4)
packages/utils/src/validators/case-validators.ts (1)
CASE_VALIDATORS(23-40)packages/project-builder-lib/src/schema/creator/schema-creator.ts (1)
definitionSchema(8-12)packages/project-builder-lib/src/schema/template-extractor/index.ts (1)
templateExtractorSchema(3-18)packages/project-builder-lib/src/schema/features/theme.ts (1)
themeSchema(46-55)
packages/project-builder-lib/src/parser/parser.ts (2)
packages/project-builder-lib/src/plugins/schema/parser.ts (3)
ZodPluginWrapper(30-65)pluginStore(53-55)zPluginWrapper(67-72)packages/project-builder-lib/src/schema/project-definition.ts (2)
ProjectDefinitionSchema(60-62)createProjectDefinitionSchema(38-50)
🔇 Additional comments (60)
packages/project-builder-lib/src/references/types.ts (1)
9-11: Trivial formatting update
The added blank line after theprefixproperty declaration is purely stylistic and aids readability by visually separating property fields from the constructor’s documentation. No functional change detected.packages/project-builder-lib/src/schema/creator/index.ts (1)
1-1: LGTM! Clean re-export pattern.This follows standard TypeScript module organization patterns for consolidating exports.
packages/utils/src/validators/index.ts (1)
3-3: LGTM! Consistent export addition.The new export follows the established pattern and maintains proper module organization.
packages/project-builder-web/src/pages/apps/edit.page.tsx (1)
54-54:Details
✅ Verification successful
Verify migration compatibility for schema restructuring.
The change correctly aligns with the new nested settings structure. Ensure that the migration system properly handles the transition to prevent runtime errors when accessing
definition.settings.general.packageScopebefore migration 13 is applied.
🏁 Script executed:
#!/bin/bash # Description: Verify that the migration system handles the schema transition properly # Expected: Find evidence that migrations run before UI code accesses the new structure # Search for migration execution in the project initialization rg -A 5 -B 5 "runSchemaMigrations|SCHEMA_MIGRATIONS" --type ts # Check for any defensive checks around settings.general access rg -A 3 -B 3 "settings\.general" --type tsLength of output: 21189
Migration handling confirmed – ready to merge.
We’ve verified that migration 013 (“moveGeneralSettings”) is included in SCHEMA_MIGRATIONS and that runSchemaMigrations is invoked before any UI code reads settings.general, ensuring the nested structure always exists:
• In packages/project-builder-web/src/app/ProjectDefinitionProvider/services/parse-project-definition-contents.ts, runSchemaMigrations(projectDefinition) is called and its migratedDefinition is passed into the React context.
• In the server, runSchemaMigrations is invoked in packages/project-builder-server/src/sync/build-project.ts before writing or using the project JSON.No additional defensive checks are needed.
packages/project-builder-lib/src/migrations/index.ts (2)
12-12: LGTM! Correct migration import.The import path and naming follow the established migration pattern.
23-23: LGTM! Proper migration ordering.The migration is correctly added at the end of the array, maintaining version order. This ensures migration 13 will run after all previous migrations, which is essential for the schema restructuring.
packages/project-builder-lib/src/schema/index.ts (1)
3-3: LGTM! Clean addition of new schema exports.The new exports for
creatorandsettingsmodules follow the established pattern and support the broader schema refactoring effort described in the PR.Also applies to: 8-8
packages/project-builder-web/src/pages/settings/theme-builder.tsx (2)
45-46: Correct implementation of nested theme settings access.The change to access theme configuration via
definition.settings.themeis consistent with the schema restructuring and properly updates the memoization dependency.
57-57:Details
✅ Verification successful
Verify consistency of theme settings updates across the codebase.
The update to use
draftConfig.settings.themecorrectly matches the new nested structure. Ensure all other theme-related code has been updated to use the same nested path.
🏁 Script executed:
#!/bin/bash # Description: Verify all theme configuration access uses the new nested settings path # Test: Search for any remaining direct theme access patterns that might need updating echo "Searching for potential outdated theme access patterns..." rg -n "\.theme\s*=" --type ts --type tsx | grep -v "settings\.theme" rg -n "definition\.theme" --type ts --type tsx | grep -v "settings\.theme" echo "Searching for correct nested theme access patterns..." rg -n "settings\.theme" --type ts --type tsxLength of output: 520
To ensure we catch all
.tsand.tsxfiles without relying onrg’s type system, let’s re-run the search using glob patterns:
🏁 Script executed:
#!/bin/bash # Description: Verify all theme configuration access uses the new nested settings path echo "Searching for direct theme assignments outside of settings.theme..." rg -n "\.theme\s*=" --glob '*.ts*' | grep -v "settings\.theme" echo "Searching for definition.theme usages..." rg -n "definition\.theme" --glob '*.ts*' | grep -v "settings\.theme" echo "Searching for any .theme property access excluding settings.theme..." rg -n "\.theme" --glob '*.ts*' | grep -v "settings\.theme"Length of output: 681
Only legacy migration uses a top-level
themepropertyAll
.themereferences outside ofsettings.themehave been updated to the new nested path, except for the one occurrence in the migration script, which intentionally reads the old config format:
- packages/project-builder-lib/src/migrations/migration-010-hex-to-oklch.ts:22 —
if (!config.theme) { … }No further changes are needed.
packages/project-builder-web/src/app/AppLayout/AppBreadcrumbs.tsx (2)
52-52: Good refactoring with the extracted projectName variable.Extracting
projectNamefrom the nestedsettings.general.namepath improves readability and avoids repetition while correctly implementing the new schema structure.
65-65: Consistent usage of the extracted projectName variable.The replacement of direct definition access with the
projectNamevariable maintains consistency and readability throughout the component.Also applies to: 99-99
packages/project-builder-cli/tests/settings.spec.ts (1)
30-30:Details
✅ Verification successful
Test correctly updated for nested settings structure.
The changes properly update the test to access project name via
settings.general.name, maintaining the test's validation logic while adapting to the new schema structure.Also applies to: 34-34
🏁 Script executed:
#!/bin/bash # Description: Verify all tests have been updated to use the new nested settings structure # Test: Search for any remaining direct name access in tests that might need updating echo "Searching for potential outdated project name access in tests..." rg -n "\.name\s*=" --type ts --glob "**/*.test.*" --glob "**/*.spec.*" | grep -v "settings\.general\.name" rg -n "projectDefinition\.name" --type ts --glob "**/*.test.*" --glob "**/*.spec.*" echo "Searching for correct nested name access in tests..." rg -n "settings\.general\.name" --type ts --glob "**/*.test.*" --glob "**/*.spec.*"Length of output: 753
✅ All tests updated for nested settings structure
Verified via search that there are no remaining direct
projectDefinition.namereferences in any.test.tsor.spec.tsfiles. All tests now correctly useprojectDefinition.settings.general.name.
No further changes needed.packages/project-builder-lib/src/plugins/spec/web-config-spec.ts (1)
3-3: LGTM! Type update aligns with plugin definition refactoring.The change from
BasePlugintoBasePluginDefinitionis consistent with the broader type refactoring mentioned in the AI summary. The nullability is preserved and the interface remains functionally equivalent.Also applies to: 11-11
packages/project-builder-server/src/compiler/backend/fastify.ts (1)
57-57:Details
✅ Verification successful
LGTM! Access paths updated for nested settings structure.
The changes correctly update the access paths to use the new
settings.generalnested structure. The logic remains functionally equivalent.Also applies to: 60-60
🏁 Script executed:
#!/bin/bash # Description: Verify all references to projectDefinition properties have been updated consistently # Expected: All direct property access should be updated to use settings.general echo "Searching for potential missed property access patterns..." rg -A 2 -B 2 "projectDefinition\.(name|portOffset|packageScope)" --type tsLength of output: 203
🏁 Script executed:
#!/bin/bash # Re-verify that no top-level projectDefinition properties are still used echo "Searching for all direct projectDefinition property accesses in TS files..." rg -n "projectDefinition\\.[A-Za-z0-9_]+" --type tsLength of output: 4538
LGTM! All nested settings access paths are consistently updated.
Confirmed no remaining direct uses of
projectDefinition.name,projectDefinition.portOffset, orprojectDefinition.packageScope—everything now correctly referencesprojectDefinition.settings.general.
No further action needed.packages/project-builder-server/src/sync/build-project.ts (1)
202-204: LGTM! Template extractor access updated for nested settings.The changes correctly update the access paths to use
projectJson.settings.templateExtractorinstead of the top-level property. The logic and functionality remain unchanged.Also applies to: 210-210
packages/project-builder-server/src/compiler/admin/index.ts (3)
71-71: Good practice: Extracting nested settings into local variables.The pattern of extracting
projectDefinition.settings.generalinto a localgeneralSettingsvariable improves code readability and reduces repetitive property access.Also applies to: 146-146
75-75: LGTM! Consistent usage of nested general settings.All property accesses have been consistently updated to use the
generalSettingsvariable, maintaining the same functionality while adapting to the new schema structure.Also applies to: 85-85, 129-129, 148-150, 153-153, 155-155
156-156:Details
✅ Verification successful
Verify the hardcoded version value.
The version is now hardcoded to
'1.0.0'. Please confirm this is intentional rather than an oversight during the migration.
🏁 Script executed:
#!/bin/bash # Description: Check if version was previously dynamic or configurable # Expected: Find references to version configuration in project settings echo "Searching for version-related configuration..." rg -A 3 -B 3 "version.*projectDefinition|projectDefinition.*version" --type tsLength of output: 183
🏁 Script executed:
#!/bin/bash # Display context around the hardcoded version in index.ts echo "---- Context around line 156 in index.ts ----" sed -n '140,180p' packages/project-builder-server/src/compiler/admin/index.ts # Find all occurrences of 'version' nearby to see if it's defined/imported elsewhere echo "---- All 'version' occurrences in this file ----" rg -n "version" -C 3 packages/project-builder-server/src/compiler/admin/index.tsLength of output: 1220
🏁 Script executed:
#!/bin/bash # Search for ProjectDefinition type definitions echo "=== interface declarations ===" rg -n "interface ProjectDefinition" -t ts echo "=== type aliases ===" rg -n "type ProjectDefinition" -t ts # Inspect the settings.general block in those definitions for f in $(rg -l "ProjectDefinition" -t ts); do echo "---- $f : settings.general snippet ----" rg -n "settings.*general" -C 5 "$f" done # Check for any default or configured version elsewhere in settings.general echo "=== Looking for 'version' in settings.general definitions ===" rg -n "general.*version" -C 3 -t tsLength of output: 48735
Hardcoded version ‘1.0.0’ is intentional
Noversionproperty exists on the ProjectDefinition settings (and no schema field for an initial version), so this literal serves as the default starting version for generated admin packages.• Location: packages/project-builder-server/src/compiler/admin/index.ts:156
packages/project-builder-server/src/compiler/backend/utils.ts (2)
10-11: LGTM! Access path updates are consistent.The changes correctly update the access paths for
portOffsetandnameto use the new nestedsettings.generalstructure. Both properties are now consistently accessed from the same nested location.
25-26: LGTM! Consistent pattern applied.The same nested access pattern is correctly applied to the Redis settings function, maintaining consistency across both utility functions.
packages/project-builder-server/src/service/builder-service.ts (1)
240-248: LGTM! Initial project definition structure correctly updated.The changes properly restructure the initial project definition to use the new nested
settings.generalformat while maintaining the requiredcliVersionandschemaVersionat the root level. Thesatisfies ProjectDefinitionInputtype annotation ensures type safety.packages/project-builder-server/src/compiler/app-entry-builder.ts (1)
60-60: LGTM! Property access correctly updated.The change correctly updates the access path for the project name to use the new nested
settings.general.namestructure, maintaining the same naming logic for the app entry.packages/project-builder-web/src/pages/settings/template-extractor.tsx (2)
33-33: LGTM! Default values access correctly updated.The change correctly updates the access path for templateExtractor configuration to use the nested
settings.templateExtractorstructure for reading default form values.
43-43: LGTM! Form submission correctly updated.The change correctly updates the save operation to write templateExtractor configuration to the nested
settings.templateExtractorlocation, maintaining consistency with the read operation.packages/project-builder-server/src/compiler/backend/index.ts (2)
44-44: LGTM: Clean extraction of general settingsGood practice to extract the frequently used nested object into a local variable for better readability and performance.
46-48: LGTM: Consistent migration to nested structureThe package name construction correctly uses the new
generalSettingsobject instead of accessing properties directly from the rootprojectDefinition. The logic remains identical.packages/project-builder-server/src/compiler/web/index.ts (3)
67-67: LGTM: Port offset migration correctly implementedThe port calculation correctly uses the new nested structure
projectDefinition.settings.general.portOffsetinstead of the direct property access. This maintains the same runtime behavior.
84-84: LGTM: Consistent pattern with backend compilerGood to see the same local variable extraction pattern used here for consistency across the codebase.
86-88: LGTM: Web compilation aligned with schema migrationAll references to general settings now correctly use the nested structure. The hardcoded version
'1.0.0'is consistent with the backend compiler changes, suggesting this is an intentional design decision.Also applies to: 91-94
packages/project-builder-web/src/pages/settings/project-settings.tsx (3)
3-3: LGTM: Schema import updated for focused validationSwitching to
generalSettingsSchemaaligns with the new nested structure and provides more focused validation for just the general settings fields.
25-27: LGTM: Form configuration correctly updatedThe form resolver and default values now work with the nested
settings.generalobject, maintaining proper validation and initialization.
33-33: LGTM: Submission path correctly updatedThe form submission correctly updates the nested
settings.generalpath instead of overwriting the entire definition.packages/project-builder-lib/src/definition/plugins/plugin-utils.ts (2)
7-7: LGTM: Import added for refined plugin typeThe addition of
BasePluginDefinitionimport supports the type refinements in the function signatures.
15-15: LGTM: Type refinement improves specificityChanging the return types from
BasePlugintoBasePluginDefinitionprovides better type safety and aligns with the actual data structure being returned. The function logic remains unchanged, making this a safe type refinement.Also applies to: 26-26
packages/project-builder-lib/src/migrations/migration-013-move-general-settings.ts (3)
3-18: Well-structured migration interfaces.The
OldConfiginterface correctly captures the flat structure where general settings (name,packageScope,portOffset) are at the root level, along with optional fields liketemplateExtractorandtheme.
20-38: Properly nested new configuration structure.The
NewConfiginterface correctly represents the target structure with general settings moved tosettings.generaland optional fields relocated to thesettingsobject. Note that theversionfield is removed in the new structure, which aligns with the migration goals.
40-70: Migration logic correctly transforms the configuration.The migration properly:
- Destructures the old flat structure
- Creates the new nested
settings.generalobject- Moves optional fields to the appropriate location
- Preserves all other fields using the spread operator
The transformation logic aligns with the interface definitions and handles both required and optional fields correctly.
packages/project-builder-cli/tests/fixtures/server-fixture.test-helper.ts (2)
24-30: Test helper correctly updated for new nested structure.The function now properly nests general settings under
settings.general, which aligns with the migration schema changes. The structure matches theNewConfiginterface from the migration.
28-28:Details
❓ Verification inconclusive
Verify the port offset change is intentional.
The port offset was changed from 7000 to 9000. Please confirm this change is intentional and not a side effect of the migration.
🏁 Script executed:
#!/bin/bash # Description: Check if other test files or configurations use similar port offsets # to understand if this change is consistent across the codebase echo "Searching for port offset references in test files..." rg -A 2 -B 2 "portOffset.*[67]000" --type tsLength of output: 163
🏁 Script executed:
#!/bin/bash # Re-run search more broadly for any portOffset references and numeric offsets echo "Searching for all instances of 'portOffset' across the repository..." rg -n "portOffset" echo echo "Searching for literal '7000' occurrences across the repository..." rg -n "7000" echo echo "Searching for literal '9000' occurrences across the repository..." rg -n "9000"Length of output: 25321
Confirm intentional port offset bump in CLI test fixture
The only occurrence of
portOffset: 9000is inpackages/project-builder-cli/tests/fixtures/server-fixture.test-helper.ts(line 28). There are no other literal references to7000or9000—other defaults use3000(CLI/web tests) and5000(builder service).• Please verify that bumping this test fixture from the previous
7000offset to9000was deliberate (e.g., to avoid port collisions).
• If unintentional, revert or adjust to the correct offset.packages/utils/src/validators/transform-with-dynamic-schema.unit.test.ts (6)
6-17: Excellent test coverage for no-schema scenario.The test properly verifies that when no schema is provided, the original data is returned unchanged and no validation issues are added. Good use of mocking for the
RefinementCtx.
19-29: Well-structured transformation test.The test correctly verifies that data transformation works when a schema is provided, using a simple uppercase transformation as an example. Proper assertions ensure the transformation occurs and no issues are reported.
31-46: Comprehensive validation failure testing.The test properly captures validation issues when schema validation fails, ensuring that:
- Issues are correctly added to the context
- Path information is preserved correctly
- Error codes match expected Zod error types
48-74: Excellent integration test with dynamic schema selection.This test demonstrates real-world usage by dynamically selecting schemas based on data properties. It properly tests both success and failure cases, showing how the utility integrates with Zod's transform functionality.
76-125: Thorough testing of nested field validation.The test properly verifies that when a
valuePathis provided, validation is applied to the correct nested field. The error path assertions ensure that validation errors are correctly attributed to the nested field location.
127-149: Good edge case handling for optional fields.The test ensures that the utility gracefully handles cases where the specified
valuePathfield is undefined or missing, which is important for robust error handling.packages/project-builder-lib/src/definition/project-definition-container.ts (2)
12-16: Proper import updates for schema refactoring.The imports correctly reflect the transition from static
projectDefinitionSchemato dynamic schema creation withcreateProjectDefinitionSchemaand the new type inference utilityInferDefinitionSchema.
91-95: Method signature correctly updated for dynamic schema support.The return type now properly uses
InferDefinitionSchema<typeof createProjectDefinitionSchema>instead of the static schema type, which aligns with the broader refactoring to support plugin-aware dynamic schema creation. The implementation remains unchanged, which is correct since the method logic doesn't need to change.packages/project-builder-web/src/app/ProjectDefinitionProvider/NewProjectCard.tsx (3)
1-7: LGTM: Clean migration to GeneralSettingsInput type.The import changes properly introduce the new
GeneralSettingsInputtype andgeneralSettingsSchema, which aligns with the schema restructuring to nest general settings undersettings.general.
22-22: LGTM: Callback signature updated to match new schema.The
saveProjectcallback signature correctly changed to acceptGeneralSettingsInputinstead of the previous picked schema data, maintaining type safety with the new nested structure.
33-36: LGTM: Form configuration properly updated for nested structure.The form defaultValues and resolver are correctly updated:
defaultValuesnow accesses the nestedexistingProject?.settings.generalresolveruses the dedicatedgeneralSettingsSchemainstead of a picked schemaThis ensures proper validation and data binding with the new schema structure.
packages/project-builder-server/src/api/projects.ts (3)
11-21: LGTM: Well-designed backward compatibility schema.The
simpleProjectDefinitionSchemaproperly handles both legacy and new project definition formats:
- Optional root-level
namefor backward compatibility- Optional nested
settings.general.namefor the new structureThis design ensures smooth migration without breaking existing projects.
30-40: LGTM: Robust validation with proper error handling.The switch from raw
JSON.parsetosafeParsewith the schema provides:
- Structured validation of project definition format
- Clear error messages indicating which project directory has issues
- Detailed validation error information from Zod
This improves error diagnostics significantly.
42-48: LGTM: Comprehensive name extraction with fallback.The name extraction logic properly:
- Tries the new nested location first (
settings?.general.name)- Falls back to the legacy root location (
name)- Provides a clear error message if neither is found
This ensures projects work regardless of their schema version.
packages/project-builder-lib/src/definition/project-definition-container.test-helper.ts (3)
8-8: LGTM: Import updated for dynamic schema creation.The import correctly changes from the static
projectDefinitionSchemato the dynamiccreateProjectDefinitionSchemafactory function, aligning with the broader migration to context-aware schema creation.
18-24: LGTM: Test data structure updated for nested settings.The test project definition properly migrates the general settings to the nested structure:
name,packageScope, andportOffsetare now undersettings.general- Default values remain appropriate for testing
- Structure matches the new schema requirements
42-44: LGTM: Dynamic schema creation with plugin context.The schema creation correctly:
- Uses
createProjectDefinitionSchemawith plugin implementation store context- Maintains consistency with the parser implementation
- Provides proper plugin support for testing
packages/project-builder-lib/src/parser/parser.ts (4)
13-16: LGTM: Enhanced type imports for dynamic schema.The import correctly adds
ProjectDefinitionSchematype, which provides better type safety for the dynamic schema approach compared to the previous generictypeof projectDefinitionSchema.
22-22: LGTM: Plugin schema updated to definition schema.The change from
basePluginSchematobasePluginDefinitionSchemaaligns with the broader refactoring of plugin-related types, ensuring consistency across the codebase.
98-98: LGTM: More specific return type annotation.The return type is now properly typed as
ZodPluginWrapper<ProjectDefinitionSchema>instead of the generic version, providing better type safety and IntelliSense support.
104-107: LGTM: Dynamic schema creation with plugin context.The schema creation correctly:
- Uses
createProjectDefinitionSchemawith the plugin implementation store- Wraps it with
zPluginWrapperfor plugin support- Maintains consistency with the test helper implementation
This properly supports the context-aware, dynamic schema approach.
| name: `${generalSettings.name}-${app.name}`, | ||
| packageName, | ||
| description: `Backend app for ${projectDefinition.name}`, | ||
| version: projectDefinition.version, | ||
| description: `Backend app for ${generalSettings.name}`, | ||
| version: '1.0.0', |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify hardcoded version is intentional
The name and description fields correctly use the new nested structure, but the version field is now hardcoded to '1.0.0' instead of using projectDefinition.version.
Please confirm this change is intentional and aligns with the schema migration goals.
🌐 Web query:
Is hardcoding version to '1.0.0' in project compilation a recommended practice for Node.js projects?
💡 Result:
Hardcoding the version to '1.0.0' in project compilation for Node.js projects is generally not a recommended practice. Here's why:
-
Lack of Flexibility: Hardcoding the version means every time you update your project, you'd need to manually change the version number in your build or compilation scripts. This is error-prone and inefficient.
-
Semantic Versioning: Node.js projects, like many other software projects, usually follow semantic versioning (semver). The version is typically specified in the
package.jsonfile. This allows tools and users to understand the nature of changes (major, minor, patch) based on the version number. -
Automated Version Management: Many tools and workflows exist to automate versioning, such as:
- Using
npm versioncommand which updates the version inpackage.jsonand creates a git tag. - CI/CD pipelines that automatically set or increment versions based on commits or tags.
- Versioning libraries like
standard-versionorsemantic-releasethat automate version updates and changelog generation.
- Using
-
Consistency: The version in
package.jsonshould be the single source of truth. Hardcoding a version elsewhere can lead to inconsistencies.
Best Practice Recommendation:
- Maintain the version in
package.json. - During your build/compilation, read the version from
package.jsondynamically rather than hardcoding it. - Use proper versioning tools and workflows to manage version increments and releases.
If you want, I can help you with a snippet or tool recommendation on how to dynamically include the version from package.json in your build process. Would you like that?
Replace hardcoded version with projectDefinition.version
Hardcoding the version to '1.0.0' prevents automated version management and can lead to drift from your source of truth. Use the version defined in your project definition (or package.json) instead.
Affected location:
- packages/project-builder-server/src/compiler/backend/index.ts (around lines 51–54)
Suggested diff:
- version: '1.0.0',
+ version: projectDefinition.version,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| name: `${generalSettings.name}-${app.name}`, | |
| packageName, | |
| description: `Backend app for ${projectDefinition.name}`, | |
| version: projectDefinition.version, | |
| description: `Backend app for ${generalSettings.name}`, | |
| version: '1.0.0', | |
| name: `${generalSettings.name}-${app.name}`, | |
| packageName, | |
| description: `Backend app for ${generalSettings.name}`, | |
| - version: '1.0.0', | |
| + version: projectDefinition.version, |
🤖 Prompt for AI Agents
In packages/project-builder-server/src/compiler/backend/index.ts around lines 51
to 54, the version field is hardcoded to '1.0.0', which is not recommended.
Replace the hardcoded string with projectDefinition.version to dynamically use
the version from your project definition or package.json, ensuring consistency
and enabling automated version management.
| export const createProjectDefinitionSchema = definitionSchema((ctx) => | ||
| z.object({ | ||
| cliVersion: z.string().nullish(), | ||
| apps: z.array(appSchema).default([]), | ||
| features: featuresSchema, | ||
| models: z.array(modelSchema).default([]), | ||
| enums: z.array(enumSchema).optional(), | ||
| isInitialized: z.boolean().default(false), | ||
| schemaVersion: z.number(), | ||
| plugins: createPluginsDefinitionSchema(ctx), | ||
| settings: createSettingsSchema(ctx), | ||
| }), | ||
| ); |
There was a problem hiding this comment.
cliVersion is nullish but .default() might express intent better
Using .nullish() allows both null and undefined, making the field effectively optional.
If only “absence” is needed, .optional() would convey the intent without permitting null:
- cliVersion: z.string().nullish(),
+ cliVersion: z.string().optional(),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export const createProjectDefinitionSchema = definitionSchema((ctx) => | |
| z.object({ | |
| cliVersion: z.string().nullish(), | |
| apps: z.array(appSchema).default([]), | |
| features: featuresSchema, | |
| models: z.array(modelSchema).default([]), | |
| enums: z.array(enumSchema).optional(), | |
| isInitialized: z.boolean().default(false), | |
| schemaVersion: z.number(), | |
| plugins: createPluginsDefinitionSchema(ctx), | |
| settings: createSettingsSchema(ctx), | |
| }), | |
| ); | |
| export const createProjectDefinitionSchema = definitionSchema((ctx) => | |
| z.object({ | |
| cliVersion: z.string().optional(), | |
| apps: z.array(appSchema).default([]), | |
| features: featuresSchema, | |
| models: z.array(modelSchema).default([]), | |
| enums: z.array(enumSchema).optional(), | |
| isInitialized: z.boolean().default(false), | |
| schemaVersion: z.number(), | |
| plugins: createPluginsDefinitionSchema(ctx), | |
| settings: createSettingsSchema(ctx), | |
| }), | |
| ); |
🤖 Prompt for AI Agents
In packages/project-builder-lib/src/schema/project-definition.ts lines 38 to 50,
replace the use of z.string().nullish() for the cliVersion field with
z.string().optional() to better express that the field is optional without
allowing null values. This change clarifies the intent by permitting only
undefined (absence) rather than both null and undefined.
💡 Verification agent
🧩 Analysis chain
settings is mandatory – legacy project files will fail validation
Older project definitions (pre-migration) do not contain a settings object.
Unless the API layer wraps this schema with a union (legacy ∨ new), parsing will now throw.
If that compatibility layer is not in place, consider:
- settings: createSettingsSchema(ctx),
+ settings: createSettingsSchema(ctx).default({
+ // minimal defaults – adjust as needed
+ general: { name: '', packageScope: '', portOffset: 1000 },
+ }),or keep the property optional during the transition period.
Understood—let’s locate where the new schema is consumed and whether a legacy‐vs‐new union exists.
🏁 Script executed:
#!/bin/bash
# 1. Find all references to the project definition schema
rg 'createProjectDefinitionSchema' -n
# 2. Check for direct .parse() invocations on that schema
rg 'createProjectDefinitionSchema.*\.parse' -n
# 3. Look for z.union (legacy ∨ new) wrapping this or related schemas
rg 'z\.union' -nLength of output: 2396
Ensure backward compatibility for missing settings in legacy projects
The new settings property on the projectDefinition schema is currently mandatory. Since older project files don’t include this field and there’s no union-based compatibility layer in the parser, any call to createProjectDefinitionSchema or its .parse() will now throw.
Please update the schema (and adjust related test-helpers) to supply defaults or make the field optional during your migration period:
Locations to update:
- packages/project-builder-lib/src/schema/project-definition.ts
- packages/project-builder-lib/src/definition/project-definition-container.test-helper.ts (and any other call sites that construct a bare project definition)
Suggested diff in project-definition.ts:
export const createProjectDefinitionSchema = definitionSchema((ctx) =>
z.object({
cliVersion: z.string().nullish(),
apps: z.array(appSchema).default([]),
features: featuresSchema,
models: z.array(modelSchema).default([]),
enums: z.array(enumSchema).optional(),
isInitialized: z.boolean().default(false),
schemaVersion: z.number(),
plugins: createPluginsDefinitionSchema(ctx),
- settings: createSettingsSchema(ctx),
+ settings: createSettingsSchema(ctx).default({
+ general: { name: '', packageScope: '', portOffset: 1000 },
+ // add other nested defaults as needed
+ }),
}),
);Or alternatively:
- settings: createSettingsSchema(ctx),
+ settings: createSettingsSchema(ctx).optional(),and handle undefined downstream.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export const createProjectDefinitionSchema = definitionSchema((ctx) => | |
| z.object({ | |
| cliVersion: z.string().nullish(), | |
| apps: z.array(appSchema).default([]), | |
| features: featuresSchema, | |
| models: z.array(modelSchema).default([]), | |
| enums: z.array(enumSchema).optional(), | |
| isInitialized: z.boolean().default(false), | |
| schemaVersion: z.number(), | |
| plugins: createPluginsDefinitionSchema(ctx), | |
| settings: createSettingsSchema(ctx), | |
| }), | |
| ); | |
| export const createProjectDefinitionSchema = definitionSchema((ctx) => | |
| z.object({ | |
| cliVersion: z.string().nullish(), | |
| apps: z.array(appSchema).default([]), | |
| features: featuresSchema, | |
| models: z.array(modelSchema).default([]), | |
| enums: z.array(enumSchema).optional(), | |
| isInitialized: z.boolean().default(false), | |
| schemaVersion: z.number(), | |
| plugins: createPluginsDefinitionSchema(ctx), | |
| settings: createSettingsSchema(ctx).default({ | |
| general: { name: '', packageScope: '', portOffset: 1000 }, | |
| // add other nested defaults as needed | |
| }), | |
| }), | |
| ); |
🤖 Prompt for AI Agents
In packages/project-builder-lib/src/schema/project-definition.ts around lines 38
to 50, the `settings` property in the project definition schema is currently
mandatory, causing validation failures for legacy project files that lack this
field. To fix this, update the schema to make `settings` optional or provide a
default value during the migration period. Additionally, update related test
helpers such as in
packages/project-builder-lib/src/definition/project-definition-container.test-helper.ts
to supply defaults or handle the absence of `settings` to maintain backward
compatibility.
1 participant
Summary by CodeRabbit
New Features
transformWithDynamicSchemaand related types.Refactor
name,packageScope,portOffset, etc.) under a nestedsettings.generalobject across the application.Bug Fixes
Chores