merge dev to main #188
merge dev to main #188
Conversation
* chore: misc changes * update * fix tests * fix lint * update test * update
Co-authored-by: ymc9 <[email protected]>
WalkthroughThe PR bumps package versions to 3.0.0-alpha.28, renames the CLI command from “validate” to “check,” removes the generate --silent option, updates CLI texts/defaults, simplifies runtime query error messages, removes the debug client option, adjusts test schemas, and trims provider-specific/other attributes from stdlib.zmodel. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
|
Claude finished @ymc9's task —— View job Pull Request ReviewReviewing PR #188 (dev → main) for code quality, bugs, performance, security, and test coverage. Todo List
Code Review SummaryThis appears to be a release PR merging development changes to main for version 3.0.0-alpha.28. The changes include version bumps, CLI refactoring, runtime improvements, and language specification updates. ✅ Positive FindingsVersion Consistency
CLI Action Improvements
Runtime Client Architecture
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/cli/src/actions/generate.ts (1)
30-40: Avoid absolute/OS-specific import path in snippetUsing ${outputPath}/schema can emit absolute paths and Windows backslashes, making the copy-paste snippet invalid. Build a normalized relative import path with forward slashes.
- console.log(colors.green(`Generation completed successfully in ${Date.now() - start}ms.\n`)); - console.log(`You can now create a ZenStack client with it. - -```ts -import { ZenStackClient } from '@zenstackhq/runtime'; -import { schema } from '${outputPath}/schema'; - -const client = new ZenStackClient(schema, { - dialect: { ... } -}); -````); + const importPath = (() => { + const abs = path.join(outputPath, 'schema'); + let rel = path.relative(process.cwd(), abs).replace(/\\/g, '/'); + if (!rel.startsWith('.')) rel = './' + rel; + return rel; + })(); + console.log(colors.green(`Generation completed successfully in ${Date.now() - start}ms.\n`)); + console.log(`You can now create a ZenStack client with it. + +```ts +import { ZenStackClient } from '@zenstackhq/runtime'; +import { schema } from '${importPath}'; + +const client = new ZenStackClient(schema, { + dialect: { ... } +}); +````);
🧹 Nitpick comments (7)
packages/cli/src/actions/check.ts (1)
16-19: Update CLI messages to say “check” instead of “validation”User-facing messages still say “validation,” which is inconsistent with the new command name.
Apply:
- console.log(colors.green('✓ Schema validation completed successfully.')); + console.log(colors.green('✓ Schema check completed successfully.')); - console.error(colors.red('✗ Schema validation failed.')); + console.error(colors.red('✗ Schema check failed.'));packages/runtime/src/client/crud/operations/base.ts (1)
170-179: Error message simplified (good); optional: include model context and avoid stringifying unknownGood move reducing PII by excluding parameters. For better diagnostics, consider including model and using (err as Error).message so we don't stringify unknown.
- const message = `Failed to execute query: ${err}, sql: ${compiled.sql}`; + const message = `Failed to execute read on model "${model}": ${(err as Error).message}, sql: ${compiled.sql}`; throw new QueryError(message, err);packages/cli/test/check.test.ts (3)
38-38: Align test names with the new “check” commandRename the suite and test titles for consistency and future grep-ability.
-describe('CLI validate command test', () => { +describe('CLI check command test', () => { @@ - it('should validate a valid schema successfully', () => { + it('should pass for a valid schema', () => { @@ - it('should fail validation for invalid schema', () => { + it('should fail for an invalid schema', () => { @@ - it('should validate schema with syntax errors', () => { + it('should detect schema syntax errors', () => {Also applies to: 39-39, 46-46, 84-84
72-72: Use fs.rmSync instead of deprecated fs.rmdirSyncfs.rmdirSync is deprecated. Use fs.rmSync for forward compatibility.
- fs.rmdirSync(path.join(workDir, 'zenstack')); + fs.rmSync(path.join(workDir, 'zenstack'), { recursive: true, force: true });
43-43: Optional: avoid shell interpolation in runCli to eliminate injection/escaping pitfallsTests are safe, but improving utils makes them more robust (handles spaces/escaping better).
Proposed change to packages/cli/test/utils.ts:
// Suggested refactor outside this file import { execFileSync } from 'node:child_process'; import path from 'node:path'; export function runCli(command: string, cwd: string) { const cli = path.join(__dirname, '../dist/index.js'); // naive split is enough for current usage; replace with a proper argv parser if needed const [cmd, ...args] = command.split(' '); execFileSync('node', [cli, cmd, ...args], { cwd, stdio: 'inherit' }); }packages/cli/src/actions/index.ts (1)
6-6: Consider a temporary deprecated alias to ease migrationIf external code imports actions.validate, a one-release deprecated alias can reduce breakage.
-import { run as check } from './check'; - -export { db, generate, info, init, migrate, check }; +import { run as check } from './check'; +// Deprecated alias to smooth the transition; remove in a future release +const validate = check; +export { db, generate, info, init, migrate, check, validate };Also applies to: 8-8
packages/cli/src/index.ts (1)
125-130: Optional: add a deprecated 'validate' alias command for smoother transitionConsider keeping a hidden/deprecated alias for one release to reduce breakage in user scripts.
program .command('check') .description('Check a ZModel schema for syntax or semantic errors.') .addOption(schemaOption) .action(checkAction); + + // Deprecated alias: remove in a future release + program + .command('validate', { hidden: false }) + .description('[DEPRECATED] Use "check" instead.') + .addOption(schemaOption) + .action(async (options) => { + console.warn(colors.yellow('"zenstack validate" is deprecated; use "zenstack check" instead.')); + await checkAction(options); + });
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (29)
package.json(1 hunks)packages/cli/package.json(1 hunks)packages/cli/src/actions/check.ts(1 hunks)packages/cli/src/actions/generate.ts(1 hunks)packages/cli/src/actions/index.ts(1 hunks)packages/cli/src/index.ts(4 hunks)packages/cli/test/check.test.ts(3 hunks)packages/common-helpers/package.json(1 hunks)packages/create-zenstack/package.json(1 hunks)packages/dialects/sql.js/package.json(1 hunks)packages/eslint-config/package.json(1 hunks)packages/ide/vscode/package.json(1 hunks)packages/language/package.json(1 hunks)packages/language/res/stdlib.zmodel(1 hunks)packages/runtime/package.json(1 hunks)packages/runtime/src/client/client-impl.ts(1 hunks)packages/runtime/src/client/crud/operations/base.ts(1 hunks)packages/runtime/src/client/executor/zenstack-query-executor.ts(1 hunks)packages/runtime/src/client/options.ts(0 hunks)packages/runtime/test/schemas/todo/schema.ts(1 hunks)packages/runtime/test/schemas/todo/todo.zmodel(1 hunks)packages/sdk/package.json(1 hunks)packages/tanstack-query/package.json(1 hunks)packages/testtools/package.json(1 hunks)packages/typescript-config/package.json(1 hunks)packages/vitest-config/package.json(1 hunks)packages/zod/package.json(1 hunks)samples/blog/package.json(1 hunks)tests/e2e/package.json(1 hunks)
💤 Files with no reviewable changes (1)
- packages/runtime/src/client/options.ts
🧰 Additional context used
📓 Path-based instructions (2)
{packages,samples,tests}/**
📄 CodeRabbit Inference Engine (CLAUDE.md)
Packages are located in
packages/,samples/, andtests/
Files:
packages/testtools/package.jsonpackages/language/package.jsonpackages/zod/package.jsonpackages/typescript-config/package.jsonsamples/blog/package.jsonpackages/cli/src/actions/check.tspackages/create-zenstack/package.jsonpackages/common-helpers/package.jsonpackages/cli/test/check.test.tspackages/cli/src/actions/index.tspackages/runtime/src/client/client-impl.tspackages/sdk/package.jsonpackages/dialects/sql.js/package.jsontests/e2e/package.jsonpackages/runtime/src/client/executor/zenstack-query-executor.tspackages/runtime/src/client/crud/operations/base.tspackages/vitest-config/package.jsonpackages/cli/package.jsonpackages/tanstack-query/package.jsonpackages/ide/vscode/package.jsonpackages/eslint-config/package.jsonpackages/runtime/package.jsonpackages/runtime/test/schemas/todo/schema.tspackages/runtime/test/schemas/todo/todo.zmodelpackages/cli/src/index.tspackages/cli/src/actions/generate.tspackages/language/res/stdlib.zmodel
tests/e2e/**
📄 CodeRabbit Inference Engine (CLAUDE.md)
E2E tests are in
tests/e2e/directory
Files:
tests/e2e/package.json
🧠 Learnings (2)
📚 Learning: 2025-08-04T08:43:33.161Z
Learnt from: CR
PR: zenstackhq/zenstack-v3#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-04T08:43:33.161Z
Learning: `zenstack generate` compiles ZModel to TypeScript schema (`schema.ts`)
Applied to files:
packages/language/package.jsonpackages/cli/src/index.tspackages/cli/src/actions/generate.ts
📚 Learning: 2025-08-04T08:43:33.161Z
Learnt from: CR
PR: zenstackhq/zenstack-v3#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-04T08:43:33.161Z
Learning: Schema is used to instantiate `ZenStackClient` with type-safe CRUD operations
Applied to files:
packages/runtime/src/client/client-impl.ts
🧬 Code Graph Analysis (2)
packages/cli/test/check.test.ts (1)
packages/cli/test/utils.ts (2)
runCli(20-23)createProject(12-18)
packages/cli/src/index.ts (2)
packages/runtime/src/client/executor/zenstack-query-executor.ts (1)
options(53-55)packages/runtime/src/client/crud/operations/base.ts (1)
options(90-92)
⏰ 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 (31)
packages/typescript-config/package.json (1)
3-3: All alpha versions and internal dependencies are aligned✅ No leftover alpha versions detected.
✅ All @zenstackhq internal dependencies use workspace:*.tests/e2e/package.json (1)
3-3: E2E tests updated: no deprecated “validate” usage
No matches forvalidateunder tests/e2e—CLI rename tocheckis correctly reflected. The version bump in tests/e2e/package.json also aligns with the new workspace release.packages/dialects/sql.js/package.json (1)
3-3: Version bump approved.No behavioral impact; aligns with monorepo release cadence.
packages/vitest-config/package.json (1)
4-4: Version bump approved.Config package remains metadata-only; no risk introduced.
packages/zod/package.json (1)
3-3: Version bump approved.No changes beyond metadata; dependencies/peers unchanged and consistent.
packages/runtime/package.json (1)
3-3: Version bump looks goodConsistent with the repo-wide alpha.28 release. No functional changes here.
packages/eslint-config/package.json (1)
3-3: LGTM on version bumpPrivate ESLint config package bumped to alpha.28; no other changes.
packages/testtools/package.json (1)
3-3: LGTM on version bumpMatches the coordinated alpha.28 release across the monorepo.
packages/common-helpers/package.json (1)
3-3: LGTM on version bumpNo API or dependency changes; aligns with the release train.
package.json (2)
3-3: Root version bump approvedRoot package updated to 3.0.0-alpha.28; consistent with the coordinated release.
3-3: All package.json versions match 3.0.0-alpha.28
✅ Verified that everypackage.jsonin the repository (root, packages, samples, tests) reports version3.0.0-alpha.28. No further action needed.packages/language/package.json (1)
4-4: Monorepo version consistency verifiedAll internal packages (@zenstackhq/*, create-zenstack, zenstack) are at 3.0.0-alpha.28 and no lingering “alpha.27” references remain.
• packages/cli through packages/zod → all at 3.0.0-alpha.28
• samples/blog, tests/e2e, root package.json → 3.0.0-alpha.28
• grep –R reported zero matches for “alpha.27”packages/cli/package.json (1)
6-6: CLI package version bump: looks good.No other changes; aligns with the repo-wide release bump.
packages/ide/vscode/package.json (1)
4-4: VS Code extension version bump: OK.Pre-release bump aligns with the rest of the workspace.
packages/create-zenstack/package.json (1)
3-3: create-zenstack version bump: OK.No functional changes in this file; consistent with the release train.
packages/sdk/package.json (1)
3-3: SDK version bump: OK.Exports and dependencies unchanged; compatible with the monorepo-wide version update.
packages/runtime/test/schemas/todo/schema.ts (1)
183-184: Verified: No tests depend on omission of thepasswordfieldWe scanned all occurrences of “password” in the repo:
- In
packages/runtime/test/utils.ts, it’s only used for the TEST_PG_PASSWORD DB connection string.- In
packages/runtime/test/schemas/todo/schema.tsand the other Z-model schemas, it appears only as a generated field definition.- No
.ts/.jstest files access.passwordon model instances or assert that it’s omitted.This change only affects the generated schema output; there’s no test in this repo that would leak or fail based on treating
passwordas a plain optionalString. Application-level code remains responsible for omitting or hashing passwords as appropriate.packages/cli/src/actions/check.ts (1)
8-10: Docstring rename to “check” looks goodThe JSDoc now matches the “check” terminology.
packages/runtime/src/client/client-impl.ts (1)
37-39: LGTM: wording tweakComment updated to “ZenStack ORM client.” reads clearer. No functional changes.
packages/tanstack-query/package.json (1)
3-3: Version bump alignedAlpha bump to 3.0.0-alpha.28 looks consistent with the repo-wide update.
samples/blog/package.json (1)
3-3: Version bump alignedSample updated to 3.0.0-alpha.28. No other changes—looks good.
packages/runtime/test/schemas/todo/todo.zmodel (1)
73-73: Password field now selectable by default – verify test and sample codeRemoving the per‐field
@omitmeanspasswordwill be returned by default. I didn’t find any direct queries against theUsermodel inpackages/runtime/test/schemas/todo, but please manually verify that no tests or sample snippets unintentionally expose this field:
Locate any calls that fetch or return a
User(e.g.findUnique,findMany,create,update).For those queries, explicitly omit the password:
client.user.findMany({ omit: { password: true } });Alternatively, enforce a model-level default omit in your schema:
model User { password String? @select(false) // … }Re-run your test suite to confirm nothing breaks. Let me know if you’d like patches to apply
omit: { password: true }across your tests.packages/runtime/src/client/executor/zenstack-query-executor.ts (1)
156-164: PII-safe error reporting LGTMSwitching to a concise message without dumping parameters removes a potential leakage path and keeps logs cleaner. Looks good.
packages/language/res/stdlib.zmodel (1)
475-480: Commenting out @@Schema directive doc is fineAcknowledging multi-schema support without exposing an active attribute aligns with the trimming effort. Looks good.
packages/cli/test/check.test.ts (1)
43-43: Command rename in tests looks goodAll invocations updated to use the new "check" command and continue asserting throw/no-throw semantics appropriately.
Also applies to: 50-50, 58-58, 65-65, 81-81, 99-99
packages/cli/src/actions/index.ts (2)
6-6: Wiring “check” action export looks correctImports/exports updated cleanly from validate → check.
Also applies to: 8-8
6-6: Verified:checkaction present and no stalevalidateimports
- packages/cli/src/actions/check.ts exists and exports
run- No
from './validate'oractions.validatereferences in the CLI source (only descriptive text in tests)packages/cli/src/index.ts (4)
28-30: LGTM: new checkAction is correctly typed and delegatedThe action signature tracks actions.check via Parameters[0], keeping types in sync.
60-62: LGTM: wording improvements for migrate/db/infoDescriptions are clearer and consistent; no behavioral impact.
Also applies to: 100-101, 107-107, 115-116
49-51: Default schema path matches resolution logic
Confirmed that the resolver first checks./zenstack/schema.zmodel, matching the documented default. No changes needed.
53-59: Verify complete removal of--silentflag
Ran a repo-wide search for--silent(usingrg -nP --hidden --glob '!.git' --glob '!**/dist/**' --glob '!**/node_modules/**' '\B--silent\b') and found no occurrences. Docs, help text, and the action parser no longer reference it.
Summary by CodeRabbit