Skip to content

Conversation

@ymc9
Copy link
Member

@ymc9 ymc9 commented Dec 18, 2025

Summary by CodeRabbit

Release Notes

  • New Features

    • SDK now exposes AST functionality as a public export, providing direct access to language AST types and utilities.
  • Documentation

    • Enhanced internal documentation annotations in ORM for API clarity.

✏️ Tip: You can customize this high-level summary in your review settings.

Copilot AI review requested due to automatic review settings December 18, 2025 12:58
@coderabbitai
Copy link

coderabbitai bot commented Dec 18, 2025

Note

Other AI code review bot(s) detected

CodeRabbit 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.

Walkthrough

This change adds documentation-only JSDoc annotations marking internal APIs as private in ORM client files, and exposes a new AST barrel export from the SDK package through package.json exports and updated build configuration.

Changes

Cohort / File(s) Summary
ORM Client Documentation Updates
packages/orm/src/client/options.ts, packages/orm/src/client/plugin.ts
Added @private JSDoc annotations to the functions property and included a "TODO: revisit" comment in plugin.ts; documentation-only changes with no behavioral impact
SDK Package Export Configuration
packages/sdk/package.json
Added new "./ast" export path with corresponding type definitions and entry points for both ESM and CommonJS formats
SDK AST Barrel Export
packages/sdk/src/ast.ts
New file providing barrel re-export of all exports from @zenstackhq/language/ast
SDK Build Configuration
packages/sdk/tsup.config.ts
Added new "ast" entry to the build configuration, mapping to src/ast.ts for compilation

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

  • Verify that the new "./ast" export paths in package.json correctly reference the generated dist files
  • Confirm that the tsup build entry "ast" correctly maps to src/ast.ts and will generate the expected dist/ast.js, dist/ast.d.ts, dist/ast.cjs, and dist/ast.d.cts files
  • Ensure the barrel re-export in ast.ts doesn't introduce circular dependencies with @zenstackhq/language/ast

Poem

🐰 Hop, hop! New exports find their way,
AST barrels bundled, ready for the day,
Documentation whispers "private" near and far,
Build paths align like stars!

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the main changes: adding @Private JSDoc annotations (typing updates) and a new ./ast export path (export updates) across ORM and SDK packages.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch chore/typing-update

📜 Recent review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 61c6b8d and b7bb26a.

📒 Files selected for processing (1)
  • packages/orm/src/client/plugin.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • packages/orm/src/client/plugin.ts
⏰ 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, sqlite)
  • GitHub Check: build-test (20.x, postgresql)

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@claude
Copy link

claude bot commented Dec 18, 2025

Claude finished @ymc9's task —— View job


PR Code Review: Minor typing and export updates

I've thoroughly reviewed this PR and the changes look good overall. Here's my detailed analysis:

Review Progress:

  • Read and analyze all 6 changed files
  • Review packages/orm/src/client/options.ts changes
  • Review packages/orm/src/client/plugin.ts changes
  • Review packages/plugins/policy/src/expression-transformer.ts changes
  • Review packages/sdk/package.json changes
  • Review new packages/sdk/src/ast.ts file
  • Review packages/sdk/tsup.config.ts changes
  • Assess overall code quality and best practices
  • Check for potential bugs or issues
  • Evaluate performance implications
  • Review security considerations
  • Assess test coverage needs
  • Provide final review summary

Code Quality Assessment

Positive Changes

  1. Better API Encapsulation (packages/orm/src/client/options.ts:54-55):

    • Added @private JSDoc annotation to functions property
    • Properly signals that this is an internal API not meant for public consumption
  2. Consistent Architecture Decision (packages/orm/src/client/plugin.ts:26-30):

    • Commented out plugin-level function support with clear TODO
    • Maintains consistency with the temporarily disabled functionality in expression-transformer.ts
  3. New AST Export Path (packages/sdk/):

    • Added proper export path for AST types via ./ast subpath
    • Follows Node.js package.json export standards with both ESM and CJS support
    • Enables cleaner imports like @zenstackhq/sdk/ast instead of deep imports

⚠️ Areas of Concern

  1. Commented Code Pattern (packages/orm/src/client/plugin.ts & packages/plugins/policy/src/expression-transformer.ts):

    • Large blocks of commented code suggest incomplete feature removal
    • The TODO comments indicate this is temporary, but without timeline or tracking issue
    • Recommendation: Consider creating a GitHub issue to track the "revisit later" items
  2. Potential Breaking Change:

    • Removing functions from RuntimePlugin interface could break existing plugin implementations
    • Mitigation: Since this is a beta version (3.0.0-beta.32), breaking changes are acceptable

🔍 Technical Details

No Issues Found With:

  • Security: No security concerns - changes are structural/API related
  • Performance: Changes are compile-time only, no runtime impact
  • Type Safety: All TypeScript types remain consistent
  • Dependencies: No new dependencies introduced

Package.json Export Structure (packages/sdk/package.json:29-38):

"./ast": {
  "import": {
    "types": "./dist/ast.d.ts",
    "default": "./dist/ast.js"
  },
  "require": {
    "types": "./dist/ast.d.cts", 
    "default": "./dist/ast.cjs"
  }
}

This follows best practices for dual ESM/CJS package exports.

🧪 Test Coverage Assessment

  • Current State: No new tests required - changes are primarily structural
  • Recommendation: If plugin function support is being temporarily disabled, ensure existing tests that rely on this functionality are updated or skipped appropriately

Summary

This is a well-executed maintenance PR that:

  • ✅ Improves API documentation and encapsulation
  • ✅ Adds a clean export path for AST types
  • ✅ Maintains architectural consistency during feature changes
  • ✅ Follows TypeScript/Node.js best practices

Approval Recommendation: ✅ APPROVE

The changes are safe, follow good practices, and maintain backward compatibility where appropriate for a beta release.


Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR makes minor typing and export updates to improve the ORM and SDK packages. The main purpose is to add a new AST export path in the SDK package and temporarily disable plugin function support while marking the feature for future revisiting.

  • Added a new /ast export subpath in the SDK package that re-exports @zenstackhq/language/ast
  • Simplified function lookup by temporarily disabling plugin-based custom function implementations
  • Marked the functions field in ClientOptions as private

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
packages/sdk/tsup.config.ts Adds AST as a new build entry point
packages/sdk/src/ast.ts Creates new file that re-exports all AST types from language package
packages/sdk/package.json Adds package.json exports configuration for the new /ast subpath
packages/plugins/policy/src/expression-transformer.ts Simplifies function lookup to only check clientOptions, comments out plugin function checking logic
packages/orm/src/client/plugin.ts Comments out the functions field from RuntimePlugin interface and removes ZModelFunction import
packages/orm/src/client/options.ts Adds @Private JSDoc annotation to the functions field

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between b240164 and 61c6b8d.

📒 Files selected for processing (6)
  • packages/orm/src/client/options.ts (1 hunks)
  • packages/orm/src/client/plugin.ts (1 hunks)
  • packages/plugins/policy/src/expression-transformer.ts (1 hunks)
  • packages/sdk/package.json (1 hunks)
  • packages/sdk/src/ast.ts (1 hunks)
  • packages/sdk/tsup.config.ts (1 hunks)
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: CR
Repo: zenstackhq/zenstack-v3 PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T01:55:04.540Z
Learning: Applies to packages/zenstackhq/orm/**/*.{ts,tsx} : Implement plugin hooks at ORM, Kysely, and entity mutation levels for query interception and customization
Learnt from: CR
Repo: zenstackhq/zenstack-v3 PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T01:55:04.540Z
Learning: Applies to packages/zenstackhq/orm/**/*.{ts,tsx} : Use Kysely as the query builder interface for low-level database queries, avoiding raw SQL when possible
📚 Learning: 2025-11-26T01:55:04.540Z
Learnt from: CR
Repo: zenstackhq/zenstack-v3 PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T01:55:04.540Z
Learning: Applies to packages/zenstackhq/orm/**/*.test.{ts,tsx} : ORM package tests should include comprehensive client API tests and policy tests

Applied to files:

  • packages/orm/src/client/options.ts
  • packages/orm/src/client/plugin.ts
📚 Learning: 2025-11-26T01:55:04.540Z
Learnt from: CR
Repo: zenstackhq/zenstack-v3 PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T01:55:04.540Z
Learning: Applies to packages/zenstackhq/orm/**/*.{ts,tsx} : Implement plugin hooks at ORM, Kysely, and entity mutation levels for query interception and customization

Applied to files:

  • packages/orm/src/client/options.ts
  • packages/orm/src/client/plugin.ts
📚 Learning: 2025-11-26T01:55:04.540Z
Learnt from: CR
Repo: zenstackhq/zenstack-v3 PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T01:55:04.540Z
Learning: Applies to packages/zenstackhq/orm/**/*.{ts,tsx} : Use Kysely as the query builder interface for low-level database queries, avoiding raw SQL when possible

Applied to files:

  • packages/orm/src/client/plugin.ts
  • packages/sdk/src/ast.ts
📚 Learning: 2025-11-26T01:55:04.540Z
Learnt from: CR
Repo: zenstackhq/zenstack-v3 PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T01:55:04.540Z
Learning: Applies to **/*.zmodel : ZModel schema files should define database structure and policies that compile to TypeScript via `zenstack generate`

Applied to files:

  • packages/orm/src/client/plugin.ts
  • packages/sdk/src/ast.ts
⏰ 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). (4)
  • GitHub Check: Agent
  • GitHub Check: build-test (20.x, sqlite)
  • GitHub Check: build-test (20.x, postgresql)
  • GitHub Check: claude-review
🔇 Additional comments (5)
packages/sdk/src/ast.ts (1)

1-1: LGTM! Clean barrel export for AST utilities.

The new export path is properly configured across package.json and tsup.config.ts, providing a clean public API for AST-related functionality.

packages/orm/src/client/options.ts (1)

53-56: JSDoc @Private annotation documents intent but doesn't enforce privacy.

The @private annotation signals that functions is internal, which aligns with restricting function resolution to clientOptions only. Note that TypeScript doesn't enforce JSDoc privacy annotations, so the property remains accessible at compile time.

packages/sdk/tsup.config.ts (1)

6-6: LGTM! Build entry correctly added for AST export.

The new ast entry properly maps to src/ast.ts, enabling the build process to generate the necessary artifacts for the ./ast export path defined in package.json.

packages/sdk/package.json (1)

29-38: LGTM! Export configuration properly defined.

The new ./ast export follows the existing pattern for the main export, correctly specifying both ESM and CommonJS entry points with their corresponding type definitions.

packages/plugins/policy/src/expression-transformer.ts (1)

556-571: Verify impact of removing plugin function resolution.

The functions property has been removed from the RuntimePlugin interface (packages/orm/src/client/plugin.ts, lines 26-30 are commented out), and the plugin function fallback logic in getFunctionImpl has been disabled (lines 558-569 are commented out). This represents an intentional behavioral change where function lookup now only checks clientOptions.functions, completely bypassing any custom functions that plugins might provide. No existing plugins in the codebase implement this functionality, but this change constrains the extensibility of the custom function system going forward. Both locations contain matching "TODO: revisit later" comments, suggesting this decision may be reconsidered.

@claude
Copy link

claude bot commented Dec 18, 2025

Claude finished @ymc9's task —— View job


PR Code Review: Minor typing and export updates

Review Progress:

  • Read and analyze all 6 changed files
  • Review packages/orm/src/client/options.ts changes
  • Review packages/orm/src/client/plugin.ts changes
  • Review packages/plugins/policy/src/expression-transformer.ts changes
  • Review packages/sdk/package.json changes
  • Review new packages/sdk/src/ast.ts file
  • Review packages/sdk/tsup.config.ts changes
  • Assess overall code quality and best practices
  • Check for potential bugs or issues
  • Evaluate performance implications
  • Review security considerations
  • Assess test coverage needs
  • Provide final review summary

Code Quality Assessment

Positive Changes

  1. Better API Encapsulation (packages/orm/src/client/options.ts:54-55):

    • Added @private JSDoc annotation to functions property
    • Properly signals that this is an internal API not meant for public consumption
  2. Consistent Architecture Decision (packages/orm/src/client/plugin.ts:26-30):

    • Commented out plugin-level function support with clear TODO
    • Maintains consistency with the temporarily disabled functionality in expression-transformer.ts
  3. New AST Export Path (packages/sdk/):

    • Added proper export path for AST types via ./ast subpath
    • Follows Node.js package.json export standards with both ESM and CJS support
    • Enables cleaner imports like @zenstackhq/sdk/ast instead of deep imports

⚠️ Areas of Concern

  1. Commented Code Pattern (packages/orm/src/client/plugin.ts & packages/plugins/policy/src/expression-transformer.ts):

    • Large blocks of commented code suggest incomplete feature removal
    • The TODO comments indicate this is temporary, but without timeline or tracking issue
    • Recommendation: Consider creating a GitHub issue to track the "revisit later" items
  2. Potential Breaking Change:

    • Removing functions from RuntimePlugin interface could break existing plugin implementations
    • Mitigation: Since this is a beta version (3.0.0-beta.32), breaking changes are acceptable

🔍 Technical Details

No Issues Found With:

  • Security: No security concerns - changes are structural/API related
  • Performance: Changes are compile-time only, no runtime impact
  • Type Safety: All TypeScript types remain consistent
  • Dependencies: No new dependencies introduced

Package.json Export Structure (packages/sdk/package.json:29-38):

"./ast": {
  "import": {
    "types": "./dist/ast.d.ts",
    "default": "./dist/ast.js"
  },
  "require": {
    "types": "./dist/ast.d.cts", 
    "default": "./dist/ast.cjs"
  }
}

This follows best practices for dual ESM/CJS package exports.

🧪 Test Coverage Assessment

  • Current State: No new tests required - changes are primarily structural
  • Recommendation: If plugin function support is being temporarily disabled, ensure existing tests that rely on this functionality are updated or skipped appropriately

Summary

This is a well-executed maintenance PR that:

  • ✅ Improves API documentation and encapsulation
  • ✅ Adds a clean export path for AST types
  • ✅ Maintains architectural consistency during feature changes
  • ✅ Follows TypeScript/Node.js best practices

Approval Recommendation: ✅ APPROVE

The changes are safe, follow good practices, and maintain backward compatibility where appropriate for a beta release.


@ymc9 ymc9 merged commit b4d0936 into dev Dec 18, 2025
5 checks passed
@ymc9 ymc9 deleted the chore/typing-update branch December 18, 2025 13:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants