fix: add regression project and optimize ts schema generation #247
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds conditional import emission for ExpressionUtils in the TS schema generator, introduces a regression test workspace with generator and configs, and adds an issue-204 regression case (schema, generated artifacts, and test). Also removes a single inline comment in a runtime test script. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Pre-merge checks (3 passed)✅ Passed checks (3 passed)
Poem
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. ✨ Finishing touches
🧪 Generate unit tests
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 |
|
Claude finished @ymc9's task —— View job Pull Request ReviewI've completed a comprehensive review of this pull request. Here's my feedback on code quality, potential issues, performance, security, and test coverage. Todo List:
Code Quality & Best Practices ✅TypeScript Schema Generator Optimizations (
|
There was a problem hiding this comment.
Pull Request Overview
This PR adds a regression testing project and optimizes TypeScript schema generation by conditionally importing the ExpressionUtils utility. The optimization reduces bundle size by only importing ExpressionUtils when it's actually used in the generated schema.
- Adds a regression testing framework with test case for issue 204
- Optimizes TS schema generation to conditionally import
ExpressionUtils - Updates comment in existing test generation script
Reviewed Changes
Copilot reviewed 11 out of 12 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| tests/regression/* | New regression testing project with configuration, test case for issue 204, and generation script |
| packages/sdk/src/ts-schema-generator.ts | Optimizes schema generation by tracking ExpressionUtils usage and conditionally importing it |
| packages/runtime/test/scripts/generate.ts | Minor comment update in existing test generation script |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (5)
tests/regression/tsconfig.json (1)
6-6: Include the root-level script for type-checking (or explicitly exclude it).
tests/regression/generate.tswon’t be type-checked with the current include globs. If you intend to type-check it, add the root pattern.- "include": ["src/**/*.ts", "test/**/*.ts"] + "include": ["*.ts", "src/**/*.ts", "test/**/*.ts"]tests/regression/test/issue-204/regression.test.ts (1)
5-11: Make the regression meaningful: replace logs with type assertions.The test currently has no assertions. Use
expectTypeOfto assert optional-array typing and removeconsole.log.-import { describe, it } from 'vitest'; +import { describe, it, expectTypeOf } from 'vitest'; @@ - it('tests issue 204', () => { - const config: Configuration = { teamColors: [ShirtColor.Black, ShirtColor.Blue] }; - console.log(config.teamColors?.[0]); - const config1: Configuration = {}; - console.log(config1); - }); + it('tests issue 204', () => { + const config: Configuration = { teamColors: [ShirtColor.Black, ShirtColor.Blue] }; + const config1: Configuration = {}; + // teamColors is optional and, when present, is an array; element access is safe-optional + expectTypeOf(config).toMatchTypeOf<Configuration>(); + expectTypeOf(config1).toMatchTypeOf<Configuration>(); + // Adjust the expected union as needed based on your TypeDef mapping + // Example expectation: + // expectTypeOf<Configuration['teamColors']>().toEqualTypeOf<ShirtColor[] | null | undefined>(); + });tests/regression/generate.ts (3)
21-21: Remove unused variable.
tsPathis never used.- const tsPath = path.join(outputDir, 'schema.ts');
24-27: Surface non-fatal warnings to aid debugging.Log
result.warningsso flaky schema issues are visible in CI.if (!result.success) { throw new Error(`Failed to load schema from ${schemaPath}: ${result.errors}`); } + if (result.warnings?.length) { + console.warn(`Loaded with warnings for ${schemaPath}:\n- ${result.warnings.join('\n- ')}`); + }
30-30: Handle top-level rejections for clean CI failures.Catch and exit on errors to avoid noisy unhandled-rejection traces.
-main(); +main().catch((err) => { + console.error(err); + process.exit(1); +});
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (11)
packages/runtime/test/scripts/generate.ts(0 hunks)packages/sdk/src/ts-schema-generator.ts(10 hunks)tests/regression/generate.ts(1 hunks)tests/regression/package.json(1 hunks)tests/regression/test/issue-204/input.ts(1 hunks)tests/regression/test/issue-204/models.ts(1 hunks)tests/regression/test/issue-204/regression.test.ts(1 hunks)tests/regression/test/issue-204/regression.zmodel(1 hunks)tests/regression/test/issue-204/schema.ts(1 hunks)tests/regression/tsconfig.json(1 hunks)tests/regression/vitest.config.ts(1 hunks)
💤 Files with no reviewable changes (1)
- packages/runtime/test/scripts/generate.ts
🧰 Additional context used
📓 Path-based instructions (2)
{packages,samples,tests}/**
📄 CodeRabbit inference engine (CLAUDE.md)
Place packages only under
packages/,samples/, ortests/
Files:
tests/regression/test/issue-204/regression.test.tstests/regression/test/issue-204/schema.tstests/regression/vitest.config.tstests/regression/test/issue-204/models.tstests/regression/generate.tstests/regression/test/issue-204/regression.zmodeltests/regression/test/issue-204/input.tstests/regression/package.jsonpackages/sdk/src/ts-schema-generator.tstests/regression/tsconfig.json
**/schema.ts
📄 CodeRabbit inference engine (CLAUDE.md)
The generated TypeScript schema should be named
schema.ts
Files:
tests/regression/test/issue-204/schema.ts
🧠 Learnings (1)
📚 Learning: 2025-09-04T12:38:14.150Z
Learnt from: CR
PR: zenstackhq/zenstack-v3#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-04T12:38:14.150Z
Learning: Applies to **/schema.ts : The generated TypeScript schema should be named `schema.ts`
Applied to files:
tests/regression/test/issue-204/schema.tstests/regression/test/issue-204/models.tstests/regression/generate.tspackages/sdk/src/ts-schema-generator.ts
🧬 Code graph analysis (4)
tests/regression/test/issue-204/regression.test.ts (1)
tests/regression/test/issue-204/models.ts (3)
Configuration(11-11)ShirtColor(12-12)ShirtColor(13-13)
tests/regression/test/issue-204/schema.ts (1)
packages/sdk/src/schema/schema.ts (1)
SchemaDef(10-18)
tests/regression/generate.ts (2)
packages/sdk/src/ts-schema-generator.ts (1)
generate(57-71)packages/language/src/index.ts (1)
loadDocument(21-132)
packages/sdk/src/ts-schema-generator.ts (2)
packages/language/src/generated/ast.ts (11)
Expression(115-115)Expression(117-117)BinaryExpr(256-262)BinaryExpr(264-264)UnaryExpr(741-746)UnaryExpr(748-748)ArrayExpr(185-189)ArrayExpr(191-191)ReferenceExpr(688-693)ReferenceExpr(695-695)isDataField(344-346)packages/sdk/src/schema/expression.ts (2)
Expression(1-10)CallExpression(46-50)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: claude-review
- GitHub Check: build-test (20.x)
🔇 Additional comments (20)
tests/regression/vitest.config.ts (1)
1-4: LGTM.Merges the shared base config with no overrides; straightforward and correct.
tests/regression/test/issue-204/regression.zmodel (1)
14-21: LGTM.The optional array
ShirtColor[]?models the issue well;Foo.configas@jsonis appropriate for the regression.packages/sdk/src/ts-schema-generator.ts (14)
55-56: LGTM: usage tracking flag.The
usedExpressionUtilsflag is a clean way to conditionally import runtime helpers.
60-62: LGTM: reset usage flag per generation.Prevents leakage across multiple runs.
90-92: LGTM: precompute schema to decide imports.Evaluating
schemaObjectfirst makes conditional import generation reliable.
134-136: LGTM:as const+satisfiesforschema.This preserves literal types and validates against
SchemaDef.
503-513: LGTM: default expressions now mark ExpressionUtils usage consistently.Routing through the helper ensures the conditional import toggles correctly.
Also applies to: 519-525
1029-1029: LGTM:_thisvia helper.Keeps usage tracking unified.
1048-1049: LGTM: member access via helper.Flattens the chain and records usage correctly.
1052-1053: LGTM:_nullvia helper.
1056-1061: LGTM: binary expression via helper.
1064-1068: LGTM: unary expression via helper.
1071-1073: LGTM: array expression via helper.
1078-1078: LGTM: field reference via helper.
1087-1093: LGTM: invocation via helper.
110-118: Verify TypeScript >= 4.7 — mixed type/value per‑specifier imports in ts-schema-generator.tspackages/sdk/src/ts-schema-generator.ts (lines 110–118) emits mixed per‑specifier type and value imports. package.json shows
"typescript": "catalog:"(inconclusive). Confirm the actual TypeScript compiler/version used by your build/CI (lockfile or installed tsc). If the project uses < 4.7, update the generator to emit type-only imports or split the import into separate type and value declarations.tests/regression/package.json (1)
10-12: Double-check runtime vs dev-time placement of @zenstackhq/runtime.Generated files import
@zenstackhq/runtime/schemaat runtime. Keeping@zenstackhq/runtimein devDependencies works in a monorepo test harness, but if this package ever runs outside test scripts, move it to dependencies.tests/regression/test/issue-204/schema.ts (1)
8-59: LGTM: generated schema matches guidelines and structure.
- File is named schema.ts as required.
- Types and enums align with the zmodel; export pattern
as const satisfies SchemaDefis correct.tests/regression/test/issue-204/models.ts (1)
8-13: LGTM: clean public surface for model/type exports.Imports and type/value exports are correct and consistent with the generated schema.
tests/regression/test/issue-204/input.ts (1)
8-30: LGTM: argument and payload type aliases look consistent.Aliases map correctly to runtime generics;
FooGetPayloadconstraint is appropriate.
|
Claude finished @ymc9's task —— View job Pull Request ReviewI've completed a comprehensive review of this pull request. Here's my feedback on code quality, potential issues, performance, security, and test coverage. Todo List:
Code Quality & Best Practices ✅TypeScript Schema Generator Optimizations (
|
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
packages/sdk/src/ts-schema-generator.ts (3)
90-92: Precomputing schemaObject before emitting imports: consider documenting or two-pass scanThis inversion works to gate imports, but it inverts the usual “imports-first” flow. Either add a brief comment here explaining the necessity, or introduce a cheap first pass to detect
ExpressionUtilsusage without building the full object.
1070-1074: Tighten return types; avoid undefined from createLiteralNodeThese helpers are expressions; returning
any(and potentiallyundefined) weakens types and risks bad nodes during refactors.Apply:
- private createArrayExpression(expr: ArrayExpr): any { + private createArrayExpression(expr: ArrayExpr): ts.Expression { return this.createExpressionUtilsCall('array', [ ts.factory.createArrayLiteralExpression(expr.items.map((item) => this.createExpression(item))), ]); } - private createRefExpression(expr: ReferenceExpr): any { + private createRefExpression(expr: ReferenceExpr): ts.Expression { if (isDataField(expr.target.ref)) { return this.createExpressionUtilsCall('field', [this.createLiteralNode(expr.target.$refText)]); } else if (isEnumField(expr.target.ref)) { return this.createLiteralExpression('StringLiteral', expr.target.$refText); } else { throw new Error(`Unsupported reference type: ${expr.target.$refText}`); } } - private createLiteralNode(arg: string | number | boolean | null): any { - return arg === null - ? ts.factory.createNull() - : typeof arg === 'string' - ? ts.factory.createStringLiteral(arg) - : typeof arg === 'number' - ? ts.factory.createNumericLiteral(arg) - : arg === true - ? ts.factory.createTrue() - : arg === false - ? ts.factory.createFalse() - : undefined; - } + private createLiteralNode(arg: string | number | boolean | null): ts.Expression { + return arg === null + ? ts.factory.createNull() + : typeof arg === 'string' + ? ts.factory.createStringLiteral(arg) + : typeof arg === 'number' + ? ts.factory.createNumericLiteral(arg) + : arg + ? ts.factory.createTrue() + : ts.factory.createFalse(); + }Also applies to: 1076-1084, 887-899
1097-1102: Bug: NumberLiteral uses Identifier instead of NumericLiteralConstructing number nodes as identifiers produces invalid TS. Use
createNumericLiteral(or parse then reusecreateLiteralNode).Apply:
- .with('NumberLiteral', () => - this.createExpressionUtilsCall('literal', [ts.factory.createIdentifier(value as string)]), - ) + .with('NumberLiteral', () => + this.createExpressionUtilsCall('literal', [ + ts.factory.createNumericLiteral(value as string), + ]), + )
🧹 Nitpick comments (1)
packages/sdk/src/ts-schema-generator.ts (1)
159-160: Use nullish coalescing for argsMinor: prefer
args ?? []to avoid future surprises if an empty array is ever intentionally passed.- args || [], + args ?? [],
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/sdk/src/ts-schema-generator.ts(10 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
{packages,samples,tests}/**
📄 CodeRabbit inference engine (CLAUDE.md)
Place packages only under
packages/,samples/, ortests/
Files:
packages/sdk/src/ts-schema-generator.ts
🧠 Learnings (1)
📚 Learning: 2025-09-04T12:38:14.150Z
Learnt from: CR
PR: zenstackhq/zenstack-v3#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-04T12:38:14.150Z
Learning: Applies to **/schema.ts : The generated TypeScript schema should be named `schema.ts`
Applied to files:
packages/sdk/src/ts-schema-generator.ts
🧬 Code graph analysis (1)
packages/sdk/src/ts-schema-generator.ts (2)
packages/language/src/generated/ast.ts (13)
Model(559-563)Model(565-565)Expression(115-115)Expression(117-117)BinaryExpr(256-262)BinaryExpr(264-264)UnaryExpr(741-746)UnaryExpr(748-748)ArrayExpr(185-189)ArrayExpr(191-191)ReferenceExpr(688-693)ReferenceExpr(695-695)isDataField(344-346)packages/sdk/src/schema/expression.ts (2)
Expression(1-10)CallExpression(46-50)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build-test (20.x)
- GitHub Check: claude-review
🔇 Additional comments (6)
packages/sdk/src/ts-schema-generator.ts (6)
55-62: Usage flag for ExpressionUtils + reset per generation: LGTMThe boolean gate is simple and effective; resetting it at the start of
generateavoids cross-run leakage.
110-118: Conditional import emission for ExpressionUtils: LGTMImporting
ExpressionUtilsonly when used keeps the generatedschema.tstrim.
154-158: Fixed callee construction: property access over string identifier is correctSwitching to
ExpressionUtils.methodviacreatePropertyAccessExpressionresolves the invalid identifier bug.
503-513: Default value mapping via ExpressionUtils (call/member): LGTMShaping defaults through the same helper ensures the usage flag is set consistently.
Also applies to: 519-525
1029-1030: _this/_null routed through ExpressionUtils: LGTMKeeps expression construction uniform.
Also applies to: 1052-1053
1048-1049: Unified ExpressionUtils path for member/binary/unary/array/field/call: LGTMThis centralization reduces drift and makes gating reliable.
Also applies to: 1056-1061, 1064-1068, 1071-1073, 1078-1079, 1087-1093
Summary by CodeRabbit
Refactor
Tests
Chores