feat(server): migrate fastify adapter #339
Conversation
|
Claude finished @ymc9's task —— View job Code Review for Fastify Adapter MigrationI've completed my review of this PR that adds Fastify adapter support to the server package. Here's my comprehensive feedback: Todo List:
✅ Strengths
🔍 Issues and RecommendationsCritical Issues
Security Concerns
Code Quality Issues
Performance Considerations
Test Coverage Assessment
🔧 Minor Improvements
📋 Recommended ActionsBefore merging:
Future improvements:
🎯 Overall AssessmentThis is a solid implementation that follows good patterns established in the codebase. The main concerns are around build configuration and error handling that need to be addressed before merging. The test coverage is good, and the abstraction with common utilities is well-designed. Recommendation: Approve after addressing critical issues 1, 2, and 4. |
WalkthroughThe PR introduces Fastify adapter support for ZenStack by extracting a common adapter options interface, implementing a new Fastify plugin that routes RPC and REST API requests, and updating existing adapters to use the shared CommonAdapterOptions. Fastify and fastify-plugin are added as optional peer dependencies with corresponding dev dependencies. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
Pull Request Overview
This PR adds Fastify adapter support to the ZenStack server package, enabling users to integrate ZenStack with Fastify applications. The implementation follows the existing adapter pattern used for Express and Next.js.
- Adds Fastify plugin implementation with support for both RPC and REST API handlers
- Refactors common adapter options into a shared interface to reduce code duplication
- Adds comprehensive test coverage for the new Fastify adapter
Reviewed Changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| pnpm-lock.yaml | Adds Fastify dependencies (fastify@5.6.1, fastify-plugin@5.1.0) and their transitive dependencies |
| packages/server/package.json | Adds Fastify as optional peer dependencies and dev dependencies |
| packages/server/src/adapter/common.ts | Creates shared CommonAdapterOptions interface to reduce duplication across adapters |
| packages/server/src/adapter/fastify/plugin.ts | Implements the Fastify plugin for handling ZenStack CRUD requests |
| packages/server/src/adapter/fastify/index.ts | Exports the Fastify plugin and its options interface |
| packages/server/src/adapter/express/middleware.ts | Refactors to extend CommonAdapterOptions instead of duplicating the interface |
| packages/server/src/adapter/next/index.ts | Refactors to extend CommonAdapterOptions instead of duplicating the interface |
| packages/server/test/adapter/fastify.test.ts | Adds comprehensive test suite for Fastify adapter with RPC and REST handlers |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. 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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/server/package.json (1)
27-51: Add export entry for the Fastify adapter.The package.json adds Fastify dependencies but is missing an export entry for the new adapter. Users won't be able to import
@zenstackhq/server/fastifywithout it.Add this export entry after the express export:
"require": { "types": "./dist/express.d.cts", "default": "./dist/express.cjs" } + }, + "./fastify": { + "import": { + "types": "./dist/fastify.d.ts", + "default": "./dist/fastify.js" + }, + "require": { + "types": "./dist/fastify.d.cts", + "default": "./dist/fastify.cjs" + } }
📜 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 (7)
packages/server/package.json(2 hunks)packages/server/src/adapter/common.ts(1 hunks)packages/server/src/adapter/express/middleware.ts(1 hunks)packages/server/src/adapter/fastify/index.ts(1 hunks)packages/server/src/adapter/fastify/plugin.ts(1 hunks)packages/server/src/adapter/next/index.ts(1 hunks)packages/server/test/adapter/fastify.test.ts(1 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/server/src/adapter/fastify/index.tspackages/server/src/adapter/next/index.tspackages/server/src/adapter/common.tspackages/server/package.jsonpackages/server/src/adapter/fastify/plugin.tspackages/server/test/adapter/fastify.test.tspackages/server/src/adapter/express/middleware.ts
🧬 Code graph analysis (4)
packages/server/src/adapter/common.ts (2)
packages/sdk/src/schema/schema.ts (1)
SchemaDef(10-18)packages/server/src/types.ts (1)
ApiHandler(67-77)
packages/server/src/adapter/fastify/plugin.ts (3)
packages/sdk/src/schema/schema.ts (1)
SchemaDef(10-18)packages/server/src/adapter/common.ts (1)
CommonAdapterOptions(7-12)packages/orm/src/client/contract.ts (1)
ClientContract(50-170)
packages/server/test/adapter/fastify.test.ts (2)
packages/testtools/src/client.ts (1)
createTestClient(52-169)packages/server/test/utils.ts (1)
makeUrl(31-33)
packages/server/src/adapter/express/middleware.ts (2)
packages/sdk/src/schema/schema.ts (1)
SchemaDef(10-18)packages/server/src/adapter/common.ts (1)
CommonAdapterOptions(7-12)
⏰ 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: Cleanup artifacts
- GitHub Check: build-test (20.x, postgresql)
- GitHub Check: build-test (20.x, sqlite)
- GitHub Check: claude-review
🔇 Additional comments (10)
packages/server/src/adapter/fastify/index.ts (1)
1-2: LGTM!Clean re-export pattern for the Fastify adapter module.
packages/server/src/adapter/common.ts (1)
1-12: LGTM!Well-designed shared interface that standardizes adapter options across Express, Fastify, and Next.js adapters.
packages/server/src/adapter/express/middleware.ts (2)
4-4: LGTM!Clean migration to use the shared CommonAdapterOptions interface.
9-25: LGTM!The interface properly extends CommonAdapterOptions while adding Express-specific options (getClient and sendResponse). This maintains backward compatibility while standardizing the base options.
packages/server/src/adapter/next/index.ts (2)
5-5: LGTM!Clean migration to the shared CommonAdapterOptions interface.
12-37: LGTM!Both Next.js adapter interfaces properly extend CommonAdapterOptions while adding their Next.js-specific options. This maintains consistency across adapters.
packages/server/test/adapter/fastify.test.ts (3)
1-7: LGTM!Clean imports and proper use of test utilities.
8-109: LGTM!Comprehensive test coverage for the RPC handler path including:
- Basic CRUD operations (create, read, update, delete)
- Query filtering
- Aggregations (count, aggregate, groupBy)
- Relations (nested creates)
- Error handling (invalid paths and malformed queries)
141-198: LGTM!Thorough test coverage for the REST handler path with proper JSON:API format validation and filtering tests.
packages/server/src/adapter/fastify/plugin.ts (1)
55-57: LGTM!Proper use of fastify-plugin to wrap the plugin handler, allowing it to be registered with proper encapsulation behavior.
|
Claude finished @ymc9's task —— View job Code Review for Fastify Adapter MigrationI've completed my review of this PR that adds Fastify adapter support to the server package. Here's my comprehensive feedback: Todo List:
✅ Strengths
🔍 Issues and RecommendationsCritical Issues
Security Concerns
Code Quality Issues
Performance Considerations
Test Coverage Assessment
🔧 Minor Improvements
📋 Recommended ActionsBefore merging:
Future improvements:
🎯 Overall AssessmentThis is a solid implementation that follows good patterns established in the codebase. The main concerns are around build configuration and error handling that need to be addressed before merging. The test coverage is good, and the abstraction with common utilities is well-designed. Recommendation: Approve after addressing critical issues 1, 2, and 4. |
Summary by CodeRabbit
New Features
Chores
Tests