refactor: remove import from orm package in generated schema#2387
Conversation
- replace "@zenstackhq/orm" dependency with "@zenstackhq/schema" - add "--generate-models" and "--generate-input" switches to CLI to control what ts files are generated
|
Claude finished @ymc9's task —— View job Code Review AnalysisTodo List
Code Review Feedback✅ Positive Aspects
🔍 Areas for ConsiderationBreaking Change Risk
Type Safety
Dependency Management
🏗️ Implementation QualityCLI Integration (packages/cli/src/index.ts:97-98).addOption(triStateBooleanOption('--generate-models [boolean]', 'generate models.ts file, defaults to true'))
.addOption(triStateBooleanOption('--generate-input [boolean]', 'generate input.ts file, defaults to true'))
Plugin Configuration Override (packages/cli/src/actions/generate.ts:192-197)if (options.generateModels !== undefined) {
pluginOptions['generateModels'] = options.generateModels;
}
if (options.generateInput !== undefined) {
pluginOptions['generateInput'] = options.generateInput;
}
Schema Generator Logic (packages/sdk/src/ts-schema-generator.ts:77-84)// the model types
if (options.generateModelTypes !== false) {
this.generateModelsAndTypeDefs(model, options);
}
// the input types
if (options.generateInputTypes !== false) {
this.generateInputTypes(model, options);
}
🚨 Potential Issues
📋 SummaryThis is a well-executed refactoring that improves the architecture by:
The implementation follows good practices with proper option handling, precedence rules, and type safety. The breaking change appears intentional as part of the v3 architectural improvements. Recommendation: ✅ Approve - This PR successfully implements the intended refactoring with good code quality and comprehensive testing. |
|
No actionable comments were generated in the recent review. 🎉 📝 WalkthroughWalkthroughMakes Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/sdk/src/ts-schema-generator.ts (1)
124-145:⚠️ Potential issue | 🔴 CriticalBoth exports verified in
@zenstackhq/schema, but migration is incomplete across the codebase.The target package correctly exports both
SchemaDef(type) andExpressionUtils(const) via its index entry point. However, many other files still import from the old path@zenstackhq/orm/schema(packages/zod, packages/testtools, packages/server, packages/plugins/policy). Update all remaining imports to use@zenstackhq/schemafor consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/sdk/src/ts-schema-generator.ts` around lines 124 - 145, Search the repo for any import declarations referencing the old module specifier '@zenstackhq/orm/schema' and replace them with '@zenstackhq/schema'; ensure imports bring in the type-only SchemaDef and the const ExpressionUtils the same way as the schemaImportDecl pattern in ts-schema-generator.ts (use a type-only import for SchemaDef and include ExpressionUtils only when used, guided by the usedExpressionUtils flag/logic), updating import specifiers across all modules that still reference the old path so they consistently import SchemaDef and ExpressionUtils from '@zenstackhq/schema'.
🧹 Nitpick comments (1)
packages/cli/src/plugins/typescript.ts (1)
31-44: Naming mismatch between plugin option keys and generator option names.The plugin reads
pluginOptions['generateModels']/pluginOptions['generateInput']but passes them asgenerateModelTypes/generateInputTypesto the generator. While functionally correct, the suffix difference (ModelsvsModelTypes,InputvsInputTypes) could confuse future maintainers. Consider aligning the names across the chain — either rename theTsSchemaGeneratorOptionsfields togenerateModels/generateInput, or usegenerateModelTypes/generateInputTypesconsistently everywhere.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/plugins/typescript.ts` around lines 31 - 44, The plugin maps pluginOptions['generateModels'] and pluginOptions['generateInput'] to local variables named generateModelTypes and generateInputTypes and passes them into TsSchemaGenerator.generate, causing a naming mismatch; update the code so the option names are consistent across the chain—either rename the TsSchemaGeneratorOptions fields to generateModels and generateInput (preferred for minimal change in the plugin) or rename the plugin's local variables and the pluginOptions keys to generateModelTypes and generateInputTypes (if you prefer the more explicit “Types” suffix); make the change in the TsSchemaGenerator class/interface (constructor/options type) and in the call site inside TsSchemaGenerator().generate so both sides use the same identifiers (refer to pluginOptions, generateModelTypes/generateInputTypes, and TsSchemaGenerator.generate).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/regression/test/issue-422/schema.ts`:
- Line 8: Add the missing `@zenstackhq/schema` dependency to the regression test
package: update tests/regression/package.json by adding "@zenstackhq/schema" to
either "dependencies" or "devDependencies" (use the same version used elsewhere
in the repo or a compatible/latest version) so the imports in
tests/regression/test/issue-422/schema.ts and
tests/regression/test/issue-503/schema.ts can resolve; after adding, run
npm/yarn install for tests/regression and re-run the regression suite to confirm
compilation succeeds.
---
Outside diff comments:
In `@packages/sdk/src/ts-schema-generator.ts`:
- Around line 124-145: Search the repo for any import declarations referencing
the old module specifier '@zenstackhq/orm/schema' and replace them with
'@zenstackhq/schema'; ensure imports bring in the type-only SchemaDef and the
const ExpressionUtils the same way as the schemaImportDecl pattern in
ts-schema-generator.ts (use a type-only import for SchemaDef and include
ExpressionUtils only when used, guided by the usedExpressionUtils flag/logic),
updating import specifiers across all modules that still reference the old path
so they consistently import SchemaDef and ExpressionUtils from
'@zenstackhq/schema'.
---
Duplicate comments:
In `@tests/regression/test/issue-503/schema.ts`:
- Line 8: The test imports SchemaDef and ExpressionUtils from
"@zenstackhq/schema" but the regression test package is missing that dependency;
add "@zenstackhq/schema" to the tests/regression/package.json dependencies (or
devDependencies) so imports of SchemaDef and ExpressionUtils resolve at test
runtime, mirroring the fix applied for issue-422.
---
Nitpick comments:
In `@packages/cli/src/plugins/typescript.ts`:
- Around line 31-44: The plugin maps pluginOptions['generateModels'] and
pluginOptions['generateInput'] to local variables named generateModelTypes and
generateInputTypes and passes them into TsSchemaGenerator.generate, causing a
naming mismatch; update the code so the option names are consistent across the
chain—either rename the TsSchemaGeneratorOptions fields to generateModels and
generateInput (preferred for minimal change in the plugin) or rename the
plugin's local variables and the pluginOptions keys to generateModelTypes and
generateInputTypes (if you prefer the more explicit “Types” suffix); make the
change in the TsSchemaGenerator class/interface (constructor/options type) and
in the call site inside TsSchemaGenerator().generate so both sides use the same
identifiers (refer to pluginOptions, generateModelTypes/generateInputTypes, and
TsSchemaGenerator.generate).
|
Claude finished @ymc9's task —— View job Code Review AnalysisTodo List
Code Review Feedback✅ Positive Aspects
🔍 Areas for ConsiderationBreaking Change Risk
Type Safety
Dependency Management
🏗️ Implementation QualityCLI Integration (packages/cli/src/index.ts:97-98).addOption(triStateBooleanOption('--generate-models [boolean]', 'generate models.ts file, defaults to true'))
.addOption(triStateBooleanOption('--generate-input [boolean]', 'generate input.ts file, defaults to true'))
Plugin Configuration Override (packages/cli/src/actions/generate.ts:192-197)if (options.generateModels !== undefined) {
pluginOptions['generateModels'] = options.generateModels;
}
if (options.generateInput !== undefined) {
pluginOptions['generateInput'] = options.generateInput;
}
Schema Generator Logic (packages/sdk/src/ts-schema-generator.ts:77-84)// the model types
if (options.generateModelTypes !== false) {
this.generateModelsAndTypeDefs(model, options);
}
// the input types
if (options.generateInputTypes !== false) {
this.generateInputTypes(model, options);
}
🚨 Issues Identified
🔒 Security & Performance
📋 SummaryThis is a well-executed refactoring that improves the architecture by:
The implementation follows good practices with proper option handling, precedence rules, and type safety. The breaking change appears intentional as part of the v3 architectural improvements. Recommendation: ✅ Approve with Minor Fixes - Complete the import migration for the remaining 36 files and ensure all packages have proper dependencies before merging. |
|
Claude finished @ymc9's task —— View job Code Review AnalysisTodo List
Code Review Feedback✅ Positive Aspects
🔍 Areas for ConsiderationBreaking Change Risk
Type Safety
Dependency Management
🏗️ Implementation QualityCLI Integration (packages/cli/src/index.ts:97-98).addOption(triStateBooleanOption('--generate-models [boolean]', 'generate models.ts file, defaults to true'))
.addOption(triStateBooleanOption('--generate-input [boolean]', 'generate input.ts file, defaults to true'))
Plugin Configuration Override (packages/cli/src/actions/generate.ts:192-197)if (options.generateModels !== undefined) {
pluginOptions['generateModels'] = options.generateModels;
}
if (options.generateInput !== undefined) {
pluginOptions['generateInput'] = options.generateInput;
}
Schema Generator Logic (packages/sdk/src/ts-schema-generator.ts:77-84)// the model types
if (options.generateModelTypes !== false) {
this.generateModelsAndTypeDefs(model, options);
}
// the input types
if (options.generateInputTypes !== false) {
this.generateInputTypes(model, options);
}
🚨 Issues Identified
🔒 Security & Performance
📋 SummaryThis is a well-executed refactoring that improves the architecture by:
The implementation follows good practices with proper option handling, precedence rules, and type safety. The breaking change appears intentional as part of the v3 architectural improvements. Recommendation: ✅ Approve with Minor Fixes - Complete the import migration for the remaining 36 files and ensure all packages have proper dependencies before merging. |
|
Claude finished @ymc9's task —— View job Code Review AnalysisTodo List
Code Review Feedback✅ Positive Aspects
🔍 Areas for ConsiderationBreaking Change Impact
Type Safety ImplementationThe generateModelTypes?: boolean; // defaults to true
generateInputTypes?: boolean; // defaults to true 🏗️ Implementation Quality AnalysisCLI Integration (
|
|
Claude finished @ymc9's task —— View job Code Review AnalysisTodo List
Code Review Feedback✅ Positive Aspects
🔍 Areas for ConsiderationBreaking Change Impact
Type Safety ImplementationThe generateModelTypes?: boolean; // defaults to true
generateInputTypes?: boolean; // defaults to true 🏗️ Implementation Quality AnalysisCLI Integration (
|
1 participant
Summary by CodeRabbit
New Features
Tests
Chores
Breaking Changes