Skip to content

Conversation

@ymc9
Copy link
Member

@ymc9 ymc9 commented Nov 18, 2025

Summary by CodeRabbit

Release Notes

  • New Features
    • Added a new format CLI command to format and standardize ZModel schema files with consistent indentation and code style.
    • Schema formatting is now automatically integrated into the schema generation process, ensuring all generated schemas are consistently formatted.
    • Formatting errors are handled gracefully, preserving the original content if formatting fails.

Copilot AI review requested due to automatic review settings November 18, 2025 05:05
@coderabbitai
Copy link

coderabbitai bot commented Nov 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 PR introduces a new ZModel schema formatting feature by adding a formatDocument function to the language package, implementing a CLI format action, wiring it into the CLI command interface, applying formatting to schema generator output, and including tests for the new functionality.

Changes

Cohort / File(s) Summary
Language formatter foundation
packages/language/src/document.ts, packages/language/src/index.ts
Introduces formatDocument(content: string) function that formats ZModel content using ZModelFormatter and Langium's TextDocument.applyEdits. Exports the new function alongside existing loadDocument.
CLI format action implementation
packages/cli/src/actions/format.ts
New CLI action that reads a schema file, formats it using formatDocument, writes the result back to disk, and handles errors by logging and re-throwing.
CLI command wiring
packages/cli/src/actions/index.ts, packages/cli/src/index.ts
Adds format to the actions export list and creates a new "format" CLI subcommand that invokes the format action with schema and noVersionCheck options.
Schema generator integration
packages/auth-adapters/better-auth/src/schema-generator.ts
Wraps generated zmodel content with formatDocument in two locations, using try/catch to silently ignore formatting failures and preserve original content.
CLI format command tests
packages/cli/test/format.test.ts
New test suite with positive case (valid schema formatting) and negative case (invalid schema silent handling) using Vitest and existing test utilities.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

  • Error handling patterns: Verify consistency between schema-generator.ts (silent catch) and format.ts (error re-throw) approaches
  • formatDocument implementation: Review the TextDocument.applyEdits logic and formatting options (insertSpaces: true, tabSize: 4)
  • CLI integration: Ensure the format command follows existing patterns and the schemaOption binding is correct
  • Test coverage: Confirm both positive and negative test cases adequately cover the new functionality

Possibly related PRs

Poem

🐰 A formatter hops into the fray,
Making schemas neat and fresh each day,
With ZModelFormatter's gentle touch,
Our code looks better—isn't that much?
Format commands, now let's play! ✨

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 40.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'feat(cli): add "format" command' directly and accurately summarizes the main change—introducing a new CLI format command across multiple files.
✨ 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 feat/format

📜 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 e8f9790 and 3f00490.

📒 Files selected for processing (7)
  • packages/auth-adapters/better-auth/src/schema-generator.ts (2 hunks)
  • packages/cli/src/actions/format.ts (1 hunks)
  • packages/cli/src/actions/index.ts (1 hunks)
  • packages/cli/src/index.ts (2 hunks)
  • packages/cli/test/format.test.ts (1 hunks)
  • packages/language/src/document.ts (2 hunks)
  • packages/language/src/index.ts (1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
packages/cli/test/format.test.ts (1)
packages/cli/test/utils.ts (2)
  • createProject (12-18)
  • runCli (20-23)
packages/cli/src/actions/format.ts (2)
packages/cli/src/actions/action-utils.ts (1)
  • getSchemaFile (9-42)
packages/language/src/document.ts (1)
  • formatDocument (216-228)
packages/cli/src/index.ts (1)
packages/cli/src/telemetry.ts (1)
  • telemetry (139-139)
packages/language/src/document.ts (2)
packages/language/src/module.ts (1)
  • createZModelServices (151-153)
packages/language/src/zmodel-formatter.ts (1)
  • ZModelFormatter (7-114)
⏰ 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)
🔇 Additional comments (7)
packages/cli/src/index.ts (2)

33-35: LGTM!

The formatAction wrapper follows the established pattern used by other CLI actions and correctly integrates telemetry tracking.


152-157: LGTM!

The format command registration mirrors the structure of the check command and appropriately reuses the schemaOption and noVersionCheckOption.

packages/cli/test/format.test.ts (1)

13-23: LGTM!

The test correctly validates that formatting works on a valid schema and verifies the formatted output includes proper indentation and alignment.

packages/auth-adapters/better-auth/src/schema-generator.ts (1)

114-120: LGTM!

The formatting integration correctly uses a try-catch to gracefully handle formatting failures, falling back to unformatted content. This defensive approach ensures schema generation always succeeds even if formatting fails.

packages/cli/src/actions/index.ts (1)

1-9: LGTM!

The addition of the format export is clean and maintains alphabetical ordering of the action exports.

packages/language/src/index.ts (1)

1-1: LGTM!

The addition of formatDocument to the public API exports is clean and appropriately expands the language package's formatting capabilities.

packages/language/src/document.ts (1)

216-228: Now let me examine the ZModelFormatter and test coverage:

Now let me examine the formatter implementation and tests:

Based on my verification of the Langium architecture and best practices, I can now provide the final response.


Langium documents proceed through a lifecycle starting with the "Parsed" state when an AST is generated, and the formatter's abstract format method is called for every node in a document through the AbstractFormatter interface. The formatDocument implementation is correct—formatters work with in-memory documents created from parsed text without requiring the full document build process.

Formatting operates on the parsed AST and does not require cross-references to be resolved, which only happens later in the build process. Syntax errors will naturally fail during the parsing step, and semantic errors do not impact formatting since the formatter only manipulates syntactic structure. The current approach aligns with Langium's documented patterns and requires no changes.

Likely an incorrect or invalid review comment.


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 Nov 18, 2025

Claude finished @ymc9's task —— View job


PR Review: feat(cli): add "format" command

Reviewing the addition of a new "format" command to the ZenStack CLI.

Review Checklist

  • Read and analyze all changed files
  • Evaluate code quality and best practices
  • Check for potential bugs or issues
  • Assess performance considerations
  • Review security concerns
  • Evaluate test coverage
  • Provide comprehensive feedback

Review Summary

This PR adds a clean and well-structured format command to the ZenStack CLI. The implementation follows existing patterns and integrates properly with the codebase architecture.

✅ Strengths

Code Quality & Consistency

  • Follows existing CLI action patterns perfectly (compare with packages/cli/src/actions/check.ts:14-21)
  • Consistent error handling with proper re-throwing to maintain CLI exit codes
  • Uses established utilities like getSchemaFile() from action-utils
  • Proper integration with telemetry tracking in packages/cli/src/index.ts:33-35

Implementation Quality

  • Clean separation of concerns: CLI command in index.ts, action logic in format.ts, formatter in language package
  • Proper use of existing formatDocument function from @zenstackhq/language package
  • Good user experience with colored success/error messages using the colors library
  • Follows the established pattern of command registration and action wrapping

Testing

  • Good test coverage with both positive and negative scenarios
  • Tests validate actual formatting behavior, not just execution
  • Includes edge case for invalid schema handling

📝 Minor Observations

Test Implementation (packages/cli/test/format.test.ts:25-32)
The test "should silently ignore invalid schema" expects the command not to throw, but the current implementation in packages/cli/src/actions/format.ts:17-23 will re-throw any errors from formatDocument(). This test might be checking behavior that doesn't match the implementation.

Error Handling Consistency
The format action follows the same pattern as the check action for error handling, which is good for consistency. Both catch errors, log them with colors, and re-throw to maintain proper exit codes.

Documentation
The command description "Format a ZModel schema file" in packages/cli/src/index.ts:154 is clear and matches other command descriptions in style.

🔒 Security & Performance

Security: ✅ No security concerns

  • Uses standard file operations with proper error handling
  • No user input validation issues
  • No credential or sensitive data exposure

Performance: ✅ Good performance characteristics

  • Synchronous file operations are appropriate for CLI usage
  • Leverages existing Langium-based formatter infrastructure
  • No unnecessary async operations or resource leaks

📊 Test Coverage Assessment

The test coverage is adequate for a formatting command:

  • ✅ Tests successful formatting with content verification
  • ✅ Tests error handling for invalid schemas
  • ✅ Uses proper test utilities (createProject, runCli)
  • ⚠️ Test expectation may not match actual error behavior (see test implementation note above)

📋 Recommendations

  1. Review Test Expectation: Verify if the test at line 31 (expect(() => runCli('format', workDir)).not.toThrow()) matches the intended behavior for invalid schemas, given that the action re-throws errors.

  2. Consider Documentation: While not required for this PR, consider if the format command should be documented in user-facing docs.

🏁 Conclusion

This is a well-implemented feature that follows ZenStack's established patterns and coding standards. The code quality is high, the integration is clean, and the functionality is properly tested. The only minor concern is a potential mismatch between test expectations and actual error handling behavior for invalid schemas.

Recommendation: ✅ Approve - This PR is ready for merge after clarifying the test behavior for invalid schemas.


@claude
Copy link

claude bot commented Nov 18, 2025

Claude finished @ymc9's task —— View job


PR Review: feat(cli): add "format" command

Reviewing the addition of a new "format" command to the ZenStack CLI.

Review Checklist

  • Read and analyze all changed files
  • Evaluate code quality and best practices
  • Check for potential bugs or issues
  • Assess performance considerations
  • Review security concerns
  • Evaluate test coverage
  • Provide comprehensive feedback

✅ Overall Assessment

This is a well-implemented feature that adds formatting capabilities to the ZenStack CLI. The implementation is clean, follows existing patterns, and integrates well with the codebase architecture.

📋 Detailed Review

Code Quality & Best Practices ⭐⭐⭐⭐⭐

Strengths:

  • Clean, focused implementation following single responsibility principle
  • Proper error handling with meaningful error messages and exit codes
  • Consistent with existing CLI action patterns in the codebase
  • Good separation of concerns (CLI logic vs. formatting logic)
  • Proper use of TypeScript types

Minor improvements:

  • Consider adding JSDoc comments to the run function for consistency with other actions

Implementation Analysis

packages/cli/src/actions/format.ts

  • Clean implementation: Simple, focused function that handles the core formatting workflow
  • Good error handling: Catches formatting errors, logs them, and re-throws to maintain proper CLI exit codes
  • Proper file operations: Uses synchronous fs operations appropriately for CLI context

packages/cli/src/index.ts

  • Consistent integration: Follows exact same pattern as other CLI commands
  • Proper telemetry: Includes telemetry tracking like all other commands
  • Good command structure: Uses existing schemaOption and noVersionCheckOption patterns

packages/language/src/document.ts

  • Robust formatter function: The formatDocument function creates an isolated document context
  • Good error isolation: Uses memory:// URI to avoid file system interactions during formatting
  • Reasonable defaults: Falls back to sensible formatting options when none provided

Security Analysis

No security concerns identified:

  • File operations are properly scoped to user-specified schema files
  • Uses existing getSchemaFile() utility for path resolution
  • No arbitrary code execution or unsafe file operations
  • Input validation handled by underlying language services

Performance Considerations

Efficient implementation:

  • Synchronous file operations appropriate for CLI context
  • Memory-based document creation avoids unnecessary I/O during formatting
  • Leverages existing Langium document services efficiently
  • Minimal overhead for the formatting operation

Test Coverage ⭐⭐⭐⭐☆

Good test coverage with room for improvement:

Existing tests (packages/cli/test/format.test.ts):

  • ✅ Tests successful formatting of valid schema
  • ✅ Tests graceful handling of invalid schema
  • ✅ Verifies actual formatting output correctness

Missing test scenarios:

  • File not found errors
  • Permission errors (read/write)
  • Different schema file extensions
  • Custom schema file paths via --schema option
  • Edge cases with very large files

Integration Quality ⭐⭐⭐⭐⭐

Excellent integration:

  • Proper export/import structure in packages/cli/src/actions/index.ts
  • Clean export from packages/language/src/index.ts
  • Seamless integration with better-auth adapter (good use of formatter in schema generation)

🔍 Potential Issues

Minor Issues:

  1. Test robustness (packages/cli/test/format.test.ts:31):

    it('should silently ignore invalid schema', () => {
        expect(() => runCli('format', workDir)).not.toThrow();
    });
    • The test name suggests "silently ignore" but the implementation actually throws errors for invalid schemas
    • Consider updating test name or adding assertions about error handling behavior
  2. Error message consistency:

    • Error message "Schema formatting failed" could be more descriptive
    • Consider including the specific error details for better debugging

Enhancement Opportunities:

  1. Additional CLI options:

    • --check flag to verify formatting without modifying files
    • --diff flag to show formatting changes
    • --write flag to control whether files are modified
  2. Batch formatting:

    • Support for formatting multiple schema files
    • Glob pattern support for file selection

🎯 Recommendations

  1. Add more comprehensive tests for edge cases mentioned above
  2. Consider adding JSDoc documentation to the format action function
  3. Update test description to accurately reflect behavior with invalid schemas

✨ Highlights

  • Clean architecture: Perfect separation between CLI concerns and language services
  • Consistent patterns: Follows established conventions throughout the codebase
  • Good error handling: Maintains proper exit codes and user feedback
  • Proper integration: Works seamlessly with existing toolchain and better-auth adapter

This is a solid implementation that enhances the developer experience without introducing technical debt or security concerns.


@claude
Copy link

claude bot commented Nov 18, 2025

Claude finished @ymc9's task —— View job


PR Review: feat(cli): add "format" command

Reviewing the addition of a new "format" command to the ZenStack CLI.

Review Checklist

  • Read and analyze all changed files
  • Evaluate code quality and best practices
  • Check for potential bugs or issues
  • Assess performance considerations
  • Review security concerns
  • Evaluate test coverage
  • Provide comprehensive feedback

Review Summary

This PR adds a clean and well-structured format command to the ZenStack CLI. The implementation follows existing patterns and integrates properly with the codebase architecture.

✅ Strengths

Code Quality & Consistency

  • Follows existing CLI action patterns perfectly (compare with packages/cli/src/actions/check.ts:14-21)
  • Consistent error handling with proper re-throwing to maintain CLI exit codes
  • Uses established utilities like getSchemaFile() from action-utils
  • Proper integration with telemetry tracking in packages/cli/src/index.ts:33-35

Implementation Quality

  • Clean separation of concerns: CLI command in index.ts, action logic in format.ts, formatter in language package
  • Proper use of existing formatDocument function from @zenstackhq/language package
  • Good user experience with colored success/error messages using the colors library
  • Follows the established pattern of command registration and action wrapping

Testing

  • Good test coverage with both positive and negative scenarios
  • Tests validate actual formatting behavior, not just execution
  • Includes edge case for invalid schema handling

📝 Minor Observations

Test Implementation (packages/cli/test/format.test.ts:25-32)
The test "should silently ignore invalid schema" expects the command not to throw, but the current implementation in packages/cli/src/actions/format.ts:17-23 will re-throw any errors from formatDocument(). This test might be checking behavior that doesn't match the implementation.

Error Handling Consistency
The format action follows the same pattern as the check action for error handling, which is good for consistency. Both catch errors, log them with colors, and re-throw to maintain proper exit codes.

Documentation
The command description "Format a ZModel schema file" in packages/cli/src/index.ts:154 is clear and matches other command descriptions in style.

🔒 Security & Performance

Security: ✅ No security concerns

  • Uses standard file operations with proper error handling
  • No user input validation issues
  • No credential or sensitive data exposure

Performance: ✅ Good performance characteristics

  • Synchronous file operations are appropriate for CLI usage
  • Leverages existing Langium-based formatter infrastructure
  • No unnecessary async operations or resource leaks

📊 Test Coverage Assessment

The test coverage is adequate for a formatting command:

  • ✅ Tests successful formatting with content verification
  • ✅ Tests error handling for invalid schemas
  • ✅ Uses proper test utilities (createProject, runCli)
  • ⚠️ Test expectation may not match actual error behavior (see test implementation note above)

📋 Recommendations

  1. Review Test Expectation: Verify if the test at line 31 (expect(() => runCli('format', workDir)).not.toThrow()) matches the intended behavior for invalid schemas, given that the action re-throws errors.

  2. Consider Documentation: While not required for this PR, consider if the format command should be documented in user-facing docs.

🏁 Conclusion

This is a well-implemented feature that follows ZenStack's established patterns and coding standards. The code quality is high, the integration is clean, and the functionality is properly tested. The only minor concern is a potential mismatch between test expectations and actual error handling behavior for invalid schemas.

Recommendation: ✅ Approve - This PR is ready for merge after clarifying the test behavior for invalid schemas.


Copilot finished reviewing on behalf of ymc9 November 18, 2025 05:08
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 adds a new format command to the CLI that formats ZModel schema files. The implementation exposes a formatDocument function from the language package and integrates it into the CLI with proper error handling and test coverage.

Key changes:

  • Implements formatDocument function that uses the existing ZModelFormatter to format schema content
  • Adds a new format CLI command with schema path options
  • Includes basic test coverage for valid and invalid schema formatting

Reviewed Changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
packages/language/src/index.ts Exports the new formatDocument function
packages/language/src/document.ts Implements formatDocument function using Langium formatter services
packages/cli/src/index.ts Registers the new format command with CLI program
packages/cli/src/actions/index.ts Exports the format action (reordered alphabetically)
packages/cli/src/actions/format.ts Implements the format action that reads, formats, and writes schema files
packages/cli/test/format.test.ts Adds test coverage for the format command

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

@ymc9 ymc9 merged commit fcd5573 into dev Nov 18, 2025
6 checks passed
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