feat(server): migrate hono adapter #341
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit 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. WalkthroughAdds a Hono adapter and tests, centralizes internal error logging via a new logInternalError utility, and updates existing adapters and package manifest to include the hono dependency and optional peer metadata. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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)
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 Code Review for Hono Adapter MigrationI've completed a comprehensive review of the Hono adapter implementation. Here's my feedback: ✅ Code Quality & Best PracticesExcellent consistency with existing patterns:
Good refactoring in
🧪 Test CoverageComprehensive test suite:
🔧 Technical ImplementationProper Hono integration:
Request handling:
📦 DependenciesPackage.json changes look good:
|
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for the Hono web framework as a new adapter, extending ZenStack's multi-framework compatibility alongside existing Express, Fastify, and Elysia adapters.
- Implements a Hono adapter with middleware handler following the same pattern as other framework adapters
- Refactors error logging into a shared utility function (
logInternalError) to reduce code duplication across adapters - Adds comprehensive test coverage for both RPC and REST API handlers
Reviewed Changes
Copilot reviewed 10 out of 11 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| pnpm-lock.yaml | Added [email protected] dependency to the lock file |
| packages/server/package.json | Added hono as an optional peer dependency (^4.6.0) and dev dependency (^4.6.3) |
| packages/server/src/adapter/hono/index.ts | Barrel export for the Hono adapter |
| packages/server/src/adapter/hono/handler.ts | Core Hono middleware handler implementation with request processing logic |
| packages/server/src/adapter/common.ts | Extracted shared logInternalError utility function |
| packages/server/src/adapter/express/middleware.ts | Refactored to use shared logInternalError function |
| packages/server/src/adapter/fastify/plugin.ts | Refactored to use shared logInternalError function |
| packages/server/src/adapter/elysia/handler.ts | Refactored to use shared logInternalError function |
| packages/server/test/adapter/hono.test.ts | Test suite for Hono adapter covering RPC and REST handlers |
| packages/server/test/adapter/express.test.ts | Updated test descriptions for consistency |
| packages/server/test/adapter/fastify.test.ts | Updated test descriptions for consistency |
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: 3
📜 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 (10)
packages/server/package.json(3 hunks)packages/server/src/adapter/common.ts(2 hunks)packages/server/src/adapter/elysia/handler.ts(2 hunks)packages/server/src/adapter/express/middleware.ts(2 hunks)packages/server/src/adapter/fastify/plugin.ts(2 hunks)packages/server/src/adapter/hono/handler.ts(1 hunks)packages/server/src/adapter/hono/index.ts(1 hunks)packages/server/test/adapter/express.test.ts(2 hunks)packages/server/test/adapter/fastify.test.ts(3 hunks)packages/server/test/adapter/hono.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/test/adapter/express.test.tspackages/server/test/adapter/fastify.test.tspackages/server/src/adapter/hono/handler.tspackages/server/src/adapter/hono/index.tspackages/server/src/adapter/elysia/handler.tspackages/server/test/adapter/hono.test.tspackages/server/src/adapter/express/middleware.tspackages/server/src/adapter/fastify/plugin.tspackages/server/src/adapter/common.tspackages/server/package.json
🧬 Code graph analysis (6)
packages/server/src/adapter/hono/handler.ts (3)
packages/sdk/src/schema/schema.ts (1)
SchemaDef(10-18)packages/server/src/adapter/common.ts (2)
CommonAdapterOptions(8-13)logInternalError(15-17)packages/orm/src/client/contract.ts (1)
ClientContract(50-170)
packages/server/src/adapter/elysia/handler.ts (1)
packages/server/src/adapter/common.ts (1)
logInternalError(15-17)
packages/server/test/adapter/hono.test.ts (4)
packages/testtools/src/client.ts (1)
createTestClient(52-169)packages/server/src/adapter/hono/handler.ts (1)
createHonoHandler(18-56)packages/server/test/utils.ts (1)
makeUrl(31-33)packages/server/src/types.ts (1)
Response(52-62)
packages/server/src/adapter/express/middleware.ts (1)
packages/server/src/adapter/common.ts (1)
logInternalError(15-17)
packages/server/src/adapter/fastify/plugin.ts (1)
packages/server/src/adapter/common.ts (1)
logInternalError(15-17)
packages/server/src/adapter/common.ts (1)
packages/server/src/types.ts (1)
LogConfig(17-17)
⏰ 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: CodeQL analysis (javascript-typescript)
- GitHub Check: build-test (20.x, postgresql)
- GitHub Check: build-test (20.x, sqlite)
- GitHub Check: claude-review
🔇 Additional comments (15)
packages/server/test/adapter/express.test.ts (1)
12-12: LGTM! Test descriptions improved for consistency.The test descriptions have been updated to use consistent terminology across the test suite.
Also applies to: 151-151
packages/server/test/adapter/fastify.test.ts (1)
9-9: LGTM! Test descriptions improved for consistency.The test descriptions have been updated to align with the naming conventions used across other adapter test suites.
Also applies to: 111-111, 142-142
packages/server/test/adapter/hono.test.ts (2)
147-163: LGTM! Helper functions are well-structured.The test helper functions (
makeRequest,unmarshal,createHonoApp) are clean and appropriately scoped for the test suite.
106-106: Verify type casing for consistency.The type is specified as lowercase
'user', while the Express and Fastify REST tests use capitalized'User'. Ensure this casing is intentional and consistent with the REST API handler's expectations.packages/server/package.json (1)
77-77: LGTM! Hono dependency properly configured.The Hono dependency has been correctly added as:
- Development dependency for testing
- Optional peer dependency for consumers
Also applies to: 87-87, 106-108
packages/server/src/adapter/elysia/handler.ts (1)
4-4: LGTM! Centralized error logging adopted.The error handling now uses the shared
logInternalErrorutility for consistent error logging across adapters.Also applies to: 68-68
packages/server/src/adapter/hono/index.ts (1)
1-1: LGTM! Standard barrel export pattern.The re-export follows the established pattern used by other adapters in the codebase.
packages/server/src/adapter/express/middleware.ts (1)
4-4: LGTM! Centralized error logging adopted.The error handling now uses the shared
logInternalErrorutility, aligning with the consistent error logging approach across all adapters.Also applies to: 73-73
packages/server/src/adapter/fastify/plugin.ts (1)
5-5: LGTM! Centralized error logging adopted.The error handling now uses the shared
logInternalErrorutility, completing the consistent error logging refactor across all adapters.Also applies to: 46-46
packages/server/src/adapter/common.ts (2)
2-3: LGTM! Imports are correct.The imports support the new
logInternalErrorfunction and follow proper conventions.
15-17: LGTM! Centralized error logging is well-designed.The function appropriately handles optional logger configuration and includes stack traces when available. This provides a consistent error logging approach across adapters.
packages/server/src/adapter/hono/handler.ts (4)
1-6: LGTM! Imports are appropriate.All necessary types and utilities are imported correctly for the Hono adapter implementation.
8-16: LGTM! Interface design is clean and flexible.The interface properly extends
CommonAdapterOptionsand thegetClientcallback signature allows both synchronous and asynchronous client retrieval, which provides implementation flexibility.Note: The return type doesn't include
nullorundefined, but line 21 checks for a falsy client. Consider whethergetClientshould returnClientContract<Schema> | nullto match the runtime validation, or if the null check is defensive programming against implementation errors.
33-40: Body parsing logic is pragmatic.The implementation gracefully handles request bodies by attempting to parse JSON when a body is present and silently ignoring parse failures. This permissive approach works well for an adapter.
Note: No HTTP method or Content-Type validation is performed, which means the adapter will attempt to parse any request body as JSON regardless of the method or declared content type.
42-54: Error handling and response handling are correct; no issues found.The type assertions on line 50 are unnecessary but harmless. After verifying the API handlers (REST and RPC implementations), they guarantee that responses always have valid HTTP status codes (200, 201, 400, 404, 403, 422, 500) and JSON object bodies. Since 204/205 No Content responses and null/undefined bodies are never produced by the handlers, the assertions don't mask any runtime risks.
For consistency, consider removing the
as objectandas ContentfulStatusCodeassertions, as all other adapters (Express, Fastify, Next.js, Elysia) pass the values directly without assertions. The code functions correctly as-is, but this would align the implementation pattern across adapters.
|
Claude finished @ymc9's task —— View job Code Review for Hono Adapter MigrationI've completed a comprehensive review of the Hono adapter implementation. Here's my detailed analysis: Review Tasks
✅ Code Quality & Best PracticesExcellent consistency with existing patterns:
Good refactoring in
🧪 Test CoverageComprehensive test suite:
🔧 Technical ImplementationProper Hono integration:
Request handling:
📦 DependenciesPackage.json changes look good:
|
Summary by CodeRabbit
New Features
Tests
Improvements