merge dev to main (v3.0.0-beta.11) #319
Conversation
* fix(vscode): filter out non-file doc uri in stage change hook * update * update * update
… API (#317) * fix: orderBy issue when used with groupBy, add zod cache and $connect API * addressing PR comments
Co-authored-by: ymc9 <104139426+ymc9@users.noreply.github.com>
|
Warning Rate limit exceeded@ymc9 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 15 minutes and 9 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (8)
WalkthroughThis is a version bump PR (3.0.0-beta.10 → 3.0.0-beta.11) across the monorepo paired with concurrent runtime refactorings: stdlib function API changes (removing Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~55 minutes Rationale: While homogeneous package.json version bumps (~15 files) reduce baseline complexity, the PR introduces heterogeneous, logic-dense changes: WeakMap-based validator caching refactor with per-schema state sharing, query builder signature changes affecting multiple call sites, stdlib function removals and semantic inversions affecting public API, and adjusted query building for grouping/aggregation. Multiple files with separate reasoning required, spanning language declarations, runtime query execution, and test expectations. High density of interconnected changes in runtime layer warrants careful review. Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
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 PR Review AnalysisComprehensive review of PR #319 merging dev to main for v3.0.0-beta.11. Todo List:
Review SummaryThis is a well-structured release PR with consistent version bumps and solid functional improvements. The changes demonstrate good engineering practices with proper testing and validation. ✅ Positive AspectsVersion Consistency: All 25+ packages correctly updated to Code Quality:
Testing: New test coverage for Performance: Schema-based caching in input validation ( 🔍 Key Functional ChangesGroup-By Improvements (
Input Validation Enhancements (
Client API (
🚨 Potential ConcernsQuery Executor Complexity (
Group-By Type Conversion:
Language Module (
🛡️ Security Assessment
📊 Performance Considerations
🧪 Test Coverage
💡 Recommendations
🎯 Overall AssessmentApproval Recommended ✅ This is a solid beta release with meaningful improvements to group-by operations, input validation, and client API. The code quality is high, testing is adequate, and no significant security concerns were identified. The caching improvements should provide measurable performance benefits. |
There was a problem hiding this comment.
Pull Request Overview
This PR merges development changes into main for v3.0.0-beta.11, primarily updating version numbers across all packages from beta.10 to beta.11. Key improvements include optimizations to query execution, enhanced caching mechanisms, and test refinements.
- Version bump from 3.0.0-beta.10 to 3.0.0-beta.11 across all packages
- Query execution optimizations including conditional name mapping and simplified orderBy handling
- Schema cache refactored to use WeakMap for better memory management
- Test fixes and new connectivity tests added
Reviewed Changes
Copilot reviewed 32 out of 32 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| package.json | Root package version bump to 3.0.0-beta.11 |
| packages/*/package.json | Version updates across all package manifests |
| packages/runtime/src/client/executor/zenstack-query-executor.ts | Conditional name mapping initialization and helper method |
| packages/runtime/src/client/crud/validator/index.ts | Schema cache moved to WeakMap with proper cache key handling |
| packages/runtime/src/client/crud/operations/group-by.ts | Simplified query building by removing nested subquery |
| packages/runtime/src/client/crud/operations/aggregate.ts | Removed useDefaultIfEmpty parameter from buildOrderBy call |
| packages/runtime/src/client/crud/dialects/base-dialect.ts | Simplified buildOrderBy signature by removing useDefaultIfEmpty |
| packages/runtime/src/client/contract.ts | Added $connect method documentation |
| packages/runtime/src/client/client-impl.ts | Implemented $connect method |
| tests/e2e/orm/client-api/connect-disconnect.test.ts | New test file for connection lifecycle |
| tests/e2e/orm/client-api/find.test.ts | Added orderBy clauses for deterministic test results |
| tests/e2e/orm/client-api/group-by.test.ts | Import reordering and test expectation fixes |
| packages/language/src/utils.ts | Variable naming consistency improvement |
| packages/language/src/module.ts | Added URI scheme check for file-based documents |
| packages/language/res/stdlib.zmodel | Documentation updates and function signature clarification |
| packages/ide/vscode/package.json | VSCode extension version update to 3.0.11 |
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: 7
🧹 Nitpick comments (3)
packages/runtime/src/client/crud/validator/index.ts (2)
195-212: Deduplicate cache map initializationgetSchemaCache and setSchemaCache both lazily create the per-schema Map. Extract a small getOrCreateCacheMap() to reduce duplication and tiny branching overhead.
+ private getOrCreateCacheMap() { + let thisCache = schemaCache.get(this.schema); + if (!thisCache) { + thisCache = new Map<string, ZodType>(); + schemaCache.set(this.schema, thisCache); + } + return thisCache; + } + private getSchemaCache(cacheKey: string) { - let thisCache = schemaCache.get(this.schema); - if (!thisCache) { - thisCache = new Map<string, ZodType>(); - schemaCache.set(this.schema, thisCache); - } - return thisCache.get(cacheKey); + return this.getOrCreateCacheMap().get(cacheKey); } private setSchemaCache(cacheKey: string, schema: ZodType) { - let thisCache = schemaCache.get(this.schema); - if (!thisCache) { - thisCache = new Map<string, ZodType>(); - schemaCache.set(this.schema, thisCache); - } - return thisCache.set(cacheKey, schema); + return this.getOrCreateCacheMap().set(cacheKey, schema); }
221-231: Cache key composition is solid; remove unnecessary non-null assertionsIncluding extraValidationsEnabled and a type marker prevents collisions. stableStringify returns string, so the
!after cacheKey can be dropped.- let schema = this.getSchemaCache(cacheKey!); + let schema = this.getSchemaCache(cacheKey); if (!schema) { schema = getSchema(model, options); - this.setSchemaCache(cacheKey!, schema); + this.setSchemaCache(cacheKey, schema); }tests/e2e/orm/client-api/connect-disconnect.test.ts (1)
4-29: Good e2e coverage; consider adding idempotency/reconnect casesTest is valid and under tests/e2e/. Optionally extend to:
- call db.$connect() twice (should succeed), and
- try db.$connect() after db.$disconnect() to assert current intended behavior (either re-connects or throws).
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (32)
package.json(1 hunks)packages/cli/package.json(1 hunks)packages/common-helpers/package.json(1 hunks)packages/config/eslint-config/package.json(1 hunks)packages/config/typescript-config/package.json(1 hunks)packages/config/vitest-config/package.json(1 hunks)packages/create-zenstack/package.json(1 hunks)packages/dialects/sql.js/package.json(1 hunks)packages/ide/vscode/package.json(1 hunks)packages/language/package.json(1 hunks)packages/language/res/stdlib.zmodel(3 hunks)packages/language/src/module.ts(1 hunks)packages/language/src/utils.ts(1 hunks)packages/plugins/policy/package.json(1 hunks)packages/runtime/package.json(1 hunks)packages/runtime/src/client/client-impl.ts(1 hunks)packages/runtime/src/client/contract.ts(1 hunks)packages/runtime/src/client/crud/dialects/base-dialect.ts(3 hunks)packages/runtime/src/client/crud/operations/aggregate.ts(1 hunks)packages/runtime/src/client/crud/operations/group-by.ts(1 hunks)packages/runtime/src/client/crud/validator/index.ts(5 hunks)packages/runtime/src/client/executor/zenstack-query-executor.ts(6 hunks)packages/sdk/package.json(1 hunks)packages/tanstack-query/package.json(1 hunks)packages/testtools/package.json(1 hunks)packages/zod/package.json(1 hunks)samples/blog/package.json(1 hunks)tests/e2e/orm/client-api/connect-disconnect.test.ts(1 hunks)tests/e2e/orm/client-api/find.test.ts(5 hunks)tests/e2e/orm/client-api/group-by.test.ts(4 hunks)tests/e2e/package.json(1 hunks)tests/regression/package.json(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
{packages,samples,tests}/**
📄 CodeRabbit inference engine (CLAUDE.md)
Place packages only under
packages/,samples/, ortests/
Files:
packages/dialects/sql.js/package.jsontests/e2e/package.jsonpackages/common-helpers/package.jsonpackages/cli/package.jsonpackages/plugins/policy/package.jsonpackages/runtime/src/client/client-impl.tstests/e2e/orm/client-api/find.test.tspackages/create-zenstack/package.jsonpackages/language/src/module.tstests/regression/package.jsonpackages/language/src/utils.tspackages/ide/vscode/package.jsonpackages/config/vitest-config/package.jsonpackages/runtime/src/client/executor/zenstack-query-executor.tspackages/runtime/src/client/contract.tspackages/config/typescript-config/package.jsonpackages/config/eslint-config/package.jsonpackages/runtime/src/client/crud/operations/aggregate.tspackages/language/package.jsonpackages/tanstack-query/package.jsontests/e2e/orm/client-api/group-by.test.tspackages/runtime/package.jsontests/e2e/orm/client-api/connect-disconnect.test.tspackages/zod/package.jsonpackages/testtools/package.jsonpackages/runtime/src/client/crud/validator/index.tspackages/sdk/package.jsonsamples/blog/package.jsonpackages/runtime/src/client/crud/operations/group-by.tspackages/runtime/src/client/crud/dialects/base-dialect.tspackages/language/res/stdlib.zmodel
tests/e2e/**
📄 CodeRabbit inference engine (CLAUDE.md)
End-to-end tests must live under
tests/e2e/
Files:
tests/e2e/package.jsontests/e2e/orm/client-api/find.test.tstests/e2e/orm/client-api/group-by.test.tstests/e2e/orm/client-api/connect-disconnect.test.ts
packages/ide/vscode/package.json
📄 CodeRabbit inference engine (CLAUDE.md)
The
ide/vscodepackage is intentionally versioned differently from other packages
Files:
packages/ide/vscode/package.json
package.json
📄 CodeRabbit inference engine (CLAUDE.md)
Pin the repository package manager to
pnpm@10.12.1via thepackageManagerfield
Files:
package.json
🧠 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 packages/ide/vscode/package.json : The `ide/vscode` package is intentionally versioned differently from other packages
Applied to files:
packages/ide/vscode/package.json
🧬 Code graph analysis (5)
packages/language/src/utils.ts (1)
packages/sdk/src/model-utils.ts (1)
hasAttribute(59-64)
packages/runtime/src/client/executor/zenstack-query-executor.ts (2)
packages/runtime/src/client/executor/name-mapper.ts (1)
QueryNameMapper(28-458)packages/sdk/src/schema/schema.ts (2)
ModelDef(20-37)TypeDefDef(101-105)
tests/e2e/orm/client-api/connect-disconnect.test.ts (1)
packages/testtools/src/client.ts (1)
createTestClient(52-169)
packages/runtime/src/client/crud/validator/index.ts (2)
packages/sdk/src/schema/schema.ts (1)
SchemaDef(10-18)packages/runtime/src/client/crud/operations/base.ts (2)
schema(90-92)options(94-96)
packages/runtime/src/client/crud/operations/group-by.ts (1)
packages/runtime/src/client/crud/dialects/base-dialect.ts (1)
fieldRef(1073-1075)
⏰ 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). (3)
- GitHub Check: build-test (20.x, sqlite)
- GitHub Check: build-test (20.x, postgresql)
- GitHub Check: claude-review
🔇 Additional comments (43)
packages/runtime/src/client/executor/zenstack-query-executor.ts (4)
25-25: LGTM!The import additions for
ModelDefandTypeDefDefcorrectly support the newschemaHasMappedNameshelper method.
45-45: Excellent performance optimization!Conditionally initializing
nameMapperonly when the schema contains mapped names avoids the overhead of scanning the schema and building transformation maps when they're not needed. The optional type correctly reflects this conditional initialization.Also applies to: 57-61
256-258: LGTM! Clean and type-safe wrapper.The
processNameMappingmethod provides a safe, concise wrapper around the optionalnameMapper. Using optional chaining with nullish coalescing (?.and??) elegantly handles both cases, and the generic type parameter ensures type safety.
187-187: LGTM! Consistent usage of the safe wrapper.Both call sites correctly use
processNameMappingto handle the optionalnameMapper, ensuring no undefined reference errors can occur.Also applies to: 206-206
tests/regression/package.json (1)
3-3: Routine version bump for release 3.0.0-beta.11.Version update is consistent with the PR's release objectives. No functional changes.
packages/dialects/sql.js/package.json (1)
3-3: Routine version bump for release 3.0.0-beta.11.Version update is consistent with the PR's release objectives. No functional changes.
packages/sdk/package.json (1)
3-3: Routine version bump for release 3.0.0-beta.11.Version update is consistent with the PR's release objectives. No functional changes.
packages/plugins/policy/package.json (1)
3-3: Routine version bump for release 3.0.0-beta.11.Version update is consistent with the PR's release objectives. No functional changes.
packages/tanstack-query/package.json (1)
3-3: Routine version bump for release 3.0.0-beta.11.Version update is consistent with the PR's release objectives. No functional changes.
tests/e2e/package.json (1)
3-3: Routine version bump for release 3.0.0-beta.11.Version update is consistent with the PR's release objectives. No functional changes.
packages/cli/package.json (1)
6-6: Routine version bump for release 3.0.0-beta.11.Version update is consistent with the PR's release objectives. No functional changes.
packages/ide/vscode/package.json (1)
4-4: Version update follows intentional vscode package versioning.The VSCode extension is intentionally versioned separately from other packages (3.0.9 → 3.0.11 vs. the monorepo's beta scheme). No functional changes.
packages/runtime/package.json (1)
3-3: Version bump is correctly applied. Routine update aligned with PR objective to release 3.0.0-beta.11 across the monorepo.packages/config/typescript-config/package.json (1)
3-3: Version bump is correctly applied. Routine update aligned with PR objective to release 3.0.0-beta.11 across the monorepo.packages/zod/package.json (1)
3-3: Version bump is correctly applied. Routine update aligned with PR objective to release 3.0.0-beta.11 across the monorepo.packages/language/package.json (1)
4-4: Version bump is correctly applied. Routine update aligned with PR objective to release 3.0.0-beta.11 across the monorepo.packages/testtools/package.json (1)
3-3: Version bump is correctly applied. Routine update aligned with PR objective to release 3.0.0-beta.11 across the monorepo.packages/language/src/utils.ts (1)
445-450: Loop variable refactor is correct and idiomatic. The change from implicit iteration to Array.find() is cleaner and preserves the identical functional behavior: locate a model with @@auth attribute, or fall back to the User model. Variable naming reflects the intent more clearly.packages/config/eslint-config/package.json (1)
3-3: Version bump is correctly applied. Routine update aligned with PR objective to release 3.0.0-beta.11 across the monorepo.packages/common-helpers/package.json (1)
3-3: Version bump is correctly applied. Routine update aligned with PR objective to release 3.0.0-beta.11 across the monorepo.packages/config/vitest-config/package.json (1)
4-4: LGTM!Version bump aligns with the PR's beta.11 release objective.
packages/create-zenstack/package.json (1)
3-3: LGTM!Version bump aligns with the PR's beta.11 release objective.
tests/e2e/orm/client-api/find.test.ts (5)
186-192: LGTM!Adding explicit ordering ensures deterministic results for cursor-based pagination tests.
195-201: LGTM!Explicit ordering makes the skip + cursor combination deterministic.
221-228: LGTM!Explicit ordering ensures consistent results with multiple cursor fields.
238-246: LGTM!Explicit ordering is crucial for backward pagination (negative take) to produce consistent results.
344-358: LGTM!Adding explicit ordering to nested includes with skip/take ensures predictable test results.
package.json (1)
3-3: LGTM!Root version bump aligns with the PR's beta.11 release objective.
samples/blog/package.json (1)
3-3: LGTM!Sample version bump aligns with the PR's beta.11 release objective.
packages/language/src/module.ts (1)
106-108: LGTM!The URI scheme guard prevents
fileURLToPath(line 110) from throwing on non-file URIs likehttp://oruntitled://. Good defensive coding.tests/e2e/orm/client-api/group-by.test.ts (4)
3-4: LGTM!Import consolidation improves readability.
62-69: LGTM!Using
expect.arrayContainingis more appropriate for group-by results where exact ordering within the result set may not be guaranteed.
81-96: LGTM!Using
expect.arrayContainingmakes the test more robust to order variations while still validating the grouped content.
50-60: Expectation change verified and correct.The test expectation at line 60 is correct. The query with
skip: 1, take: -1, orderBy: { email: 'desc' }correctly returnsu3@test.com:
- After where filter (email ≠ 'u2@test.com'): [u1, u3]
- Negative take semantics: take < 0 reverses the sort direction (desc becomes asc), making the result [u1, u3]
- Skip and take applied: skip 1 → [u3], take 1 → [u3]
- Result processor reverses (since take < 0): [u3]
The groupBy operation handler at
packages/runtime/src/client/crud/operations/group-by.tsimplements this correctly, and the result is processed throughResultProcessor.processResult()which reverses the array whentake < 0.packages/language/res/stdlib.zmodel (1)
553-557: LGTM!Renaming the parameter from
regextopatternimproves clarity and reduces confusion with the function name itself.packages/runtime/src/client/crud/validator/index.ts (2)
53-54: Shared WeakMap cache looks goodUsing WeakMap<SchemaDef, Map<string, ZodType>> is appropriate to avoid leaks and share schemas across instances.
Please confirm that the same SchemaDef object instance is reused across clients so the cache can actually hit. If different (deep-cloned) objects are used, cache effectiveness will drop.
316-321: Typedef schema caching LGTMPer-schema caching keyed by typedef name and validation flag is correct; avoids rebuilding nested field schemas.
Also applies to: 343-345
packages/runtime/src/client/client-impl.ts (1)
236-241: The review comment mischaracterizes expected client behavior as design flaws.The three main concerns are incorrect:
"After $disconnect, $connect won't re-init" — This is expected, not a bug. Calling
$disconnect()invokesdestroy()on the connection pool, which is standard cleanup. The client should not be reused after disconnect, matching typical database client patterns (Prisma, etc.). No re-initialization is needed or expected."Behavior inside transactions unclear" — $connect is not in the
TRANSACTION_UNSUPPORTED_METHODSlist (['$transaction', '$disconnect', '$use']), and has noisTransactionguard because it's intentionally allowed. It usesthis.kysely.connection()which retrieves from the pool, not the transaction context, making it safe during transactions.Error normalization — A minor optional improvement, not a blocker.
The code's design is correct: $connect pings the DB within the connection lifecycle, and $disconnect properly terminates it.
Likely an incorrect or invalid review comment.
packages/runtime/src/client/crud/operations/group-by.ts (3)
14-16: LGTM! Well-structured refactoring from subquery to flat query.The change from a nested subquery approach to a flat main query with direct filtering is clear and maintains correct semantics. Using
this.modelfor both the table name and alias is appropriate since sub-aliasing has been removed.
18-18: LGTM! Simplified fieldRef usage.Removing the sub-alias parameter aligns with the flat query refactoring and correctly uses the simplified fieldRef signature.
40-40: LGTM! Correct having clause application.The having clause is correctly applied to the main query context, consistent with the flat query refactoring.
packages/runtime/src/client/crud/dialects/base-dialect.ts (2)
92-92: LGTM! Call site correctly updated to new signature.The
buildOrderBycall uses the simplified signature without theuseDefaultIfEmptyparameter. The unconditional call is appropriate sincebuildOrderBynow handles undefinedorderByby returning the query as-is.
853-853: LGTM! Relation ordering call site correctly updated.The
buildOrderBycall for to-one relation ordering correctly uses the simplified signature, passing the relation type, model alias, ordering specification, and negation flag.
* fix: make computed fields on base models work fixes #284 * update
|
Claude finished @ymc9's task —— View job PR Review AnalysisComprehensive review of PR #319 merging dev to main for v3.0.0-beta.11. Todo List:
Review SummaryThis is a well-structured release PR with consistent version bumps and solid functional improvements. The changes demonstrate good engineering practices with proper testing and validation. ✅ Positive AspectsVersion Consistency: All 25+ packages correctly updated to Code Quality:
Testing: New test coverage for Performance: Schema-based caching in input validation ( 🔍 Key Functional ChangesGroup-By Improvements (
Input Validation Enhancements (
Client API (
🚨 Potential ConcernsQuery Executor Complexity (
Group-By Type Conversion:
Language Module (
🛡️ Security Assessment
📊 Performance Considerations
🧪 Test Coverage
💡 Recommendations
🎯 Overall AssessmentApproval Recommended ✅ This is a solid beta release with meaningful improvements to group-by operations, input validation, and client API. The code quality is high, testing is adequate, and no significant security concerns were identified. The caching improvements should provide measurable performance benefits. |
|
Claude finished @ymc9's task —— View job PR Review AnalysisComprehensive review of PR #319 merging dev to main for v3.0.0-beta.11. Todo List:
Review SummaryThis is a well-structured release PR with consistent version bumps and solid functional improvements. The changes demonstrate good engineering practices with proper testing and validation. ✅ Positive AspectsVersion Consistency: All 25+ packages correctly updated to Code Quality:
Testing: New test coverage for Performance: Schema-based caching in input validation ( 🔍 Key Functional ChangesGroup-By Improvements (
Input Validation Enhancements (
Client API (
🚨 Potential ConcernsQuery Executor Complexity (
Group-By Type Conversion:
Language Module (
🛡️ Security Assessment
📊 Performance Considerations
🧪 Test Coverage
💡 Recommendations
🎯 Overall AssessmentApproval Recommended ✅ This is a solid beta release with meaningful improvements to group-by operations, input validation, and client API. The code quality is high, testing is adequate, and no significant security concerns were identified. The caching improvements should provide measurable performance benefits. |
|
Claude finished @ymc9's task —— View job PR Review AnalysisComprehensive review of PR #319 merging dev to main for v3.0.0-beta.11. Todo List:
Review SummaryThis is a well-structured release PR with consistent version bumps and solid functional improvements. The changes demonstrate good engineering practices with proper testing and validation. ✅ Positive AspectsVersion Consistency: All 25+ packages correctly updated to Code Quality:
Testing: New test coverage for Performance: Schema-based caching in input validation ( 🔍 Key Functional ChangesGroup-By Improvements (
Input Validation Enhancements (
Client API (
🚨 Potential ConcernsQuery Executor Complexity (
Group-By Type Conversion:
Language Module (
🛡️ Security Assessment
📊 Performance Considerations
🧪 Test Coverage
💡 Recommendations
🎯 Overall AssessmentApproval Recommended ✅ This is a solid beta release with meaningful improvements to group-by operations, input validation, and client API. The code quality is high, testing is adequate, and no significant security concerns were identified. The caching improvements should provide measurable performance benefits. |
Summary by CodeRabbit
New Features
$connect()method for explicit database connection validation.Breaking Changes
search()function from standard library.regex()parameter fromregextopattern.hasEvery()andhasSome()list-check functions.Chores