-
Notifications
You must be signed in to change notification settings - Fork 11
feat(server): migrate sveltekit adapter #343
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
WalkthroughThis pull request introduces a new SvelteKit adapter for the server package, applies consistent naming conventions to adapter options types across multiple adapters, replaces wildcard re-exports with explicit named exports in adapter index files, and updates the build and package configuration to support the new integration. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 |
|
Claude finished @ymc9's task —— View job Code Review: SvelteKit Adapter MigrationI've reviewed the SvelteKit adapter implementation and compared it with existing adapter patterns. Here's my comprehensive feedback: Review Tasks:
✅ Positive Aspects
🔍 Issues and RecommendationsCritical Issues
Security Concerns
Performance Considerations
Code Quality Issues
Test Issues
🔧 Recommended Fixes
📊 Overall AssessmentRating: Good (B+) This is a solid implementation that maintains consistency with the existing codebase architecture. The test coverage is comprehensive, and the code follows established patterns. The main concerns are around error handling robustness and security considerations, which are easily addressable. Recommendation: ✅ Approve with minor fixes - Address the JSON parsing vulnerability and consider the other suggestions for production readiness. |
There was a problem hiding this 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 SvelteKit adapter support to the ZenStack server package, enabling CRUD request handling for SvelteKit applications. The changes also standardize adapter option type naming across all framework adapters for consistency.
- Adds a new SvelteKit adapter with handler implementation
- Renames adapter option types to follow a consistent naming pattern (e.g.,
MiddlewareOptions→ExpressMiddlewareOptions) - Updates package exports to use explicit named exports instead of wildcard exports
Reviewed Changes
Copilot reviewed 13 out of 14 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| pnpm-lock.yaml | Adds @sveltejs/kit and its dependencies to support the new SvelteKit adapter |
| packages/server/package.json | Adds SvelteKit as an optional peer dependency and exports the new adapter entry point |
| packages/server/tsup.config.ts | Adds build configuration for the sveltekit adapter bundle |
| packages/server/src/adapter/sveltekit/index.ts | Exports the SvelteKit handler and options type |
| packages/server/src/adapter/sveltekit/handler.ts | Implements the SvelteKit request handler as a SvelteKit hooks handler |
| packages/server/test/adapter/sveltekit.test.ts | Adds comprehensive tests for both RPC and REST API handlers |
| packages/server/src/adapter/nuxt/index.ts | Changes from wildcard export to explicit named exports |
| packages/server/src/adapter/nuxt/handler.ts | Renames HandlerOptions to NuxtHandlerOptions |
| packages/server/src/adapter/hono/index.ts | Changes from wildcard export to explicit named exports |
| packages/server/src/adapter/fastify/plugin.ts | Renames PluginOptions to FastifyPluginOptions |
| packages/server/src/adapter/fastify/index.ts | Updates export to use renamed FastifyPluginOptions |
| packages/server/src/adapter/express/middleware.ts | Renames MiddlewareOptions to ExpressMiddlewareOptions |
| packages/server/src/adapter/express/index.ts | Updates export to use renamed ExpressMiddlewareOptions |
| packages/server/src/adapter/elysia/index.ts | Changes from wildcard export to explicit named exports |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/server/test/adapter/sveltekit.test.ts (1)
138-154: Helper functions look good, with one optional note.The
makeRequestandunmarshalhelpers properly simulate SvelteKit's request/response structure.Minor observation: The
useSuperJsonparameter inunmarshal(line 151) defaults tofalseand is never called withtruein the current tests. This may be intentional for future use, but if SuperJSON serialization testing isn't planned, the parameter could be removed.
📜 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 (13)
packages/server/package.json(4 hunks)packages/server/src/adapter/elysia/index.ts(1 hunks)packages/server/src/adapter/express/index.ts(1 hunks)packages/server/src/adapter/express/middleware.ts(2 hunks)packages/server/src/adapter/fastify/index.ts(1 hunks)packages/server/src/adapter/fastify/plugin.ts(2 hunks)packages/server/src/adapter/hono/index.ts(1 hunks)packages/server/src/adapter/nuxt/handler.ts(1 hunks)packages/server/src/adapter/nuxt/index.ts(1 hunks)packages/server/src/adapter/sveltekit/handler.ts(1 hunks)packages/server/src/adapter/sveltekit/index.ts(1 hunks)packages/server/test/adapter/sveltekit.test.ts(1 hunks)packages/server/tsup.config.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/express/index.tspackages/server/src/adapter/fastify/plugin.tspackages/server/tsup.config.tspackages/server/src/adapter/express/middleware.tspackages/server/src/adapter/nuxt/handler.tspackages/server/package.jsonpackages/server/src/adapter/sveltekit/handler.tspackages/server/src/adapter/elysia/index.tspackages/server/src/adapter/sveltekit/index.tspackages/server/src/adapter/hono/index.tspackages/server/src/adapter/fastify/index.tspackages/server/src/adapter/nuxt/index.tspackages/server/test/adapter/sveltekit.test.ts
🧬 Code graph analysis (5)
packages/server/src/adapter/fastify/plugin.ts (2)
packages/sdk/src/schema/schema.ts (1)
SchemaDef(10-18)packages/server/src/adapter/common.ts (1)
CommonAdapterOptions(8-13)
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(8-13)
packages/server/src/adapter/nuxt/handler.ts (3)
packages/sdk/src/schema/schema.ts (1)
SchemaDef(10-18)packages/server/src/adapter/common.ts (1)
CommonAdapterOptions(8-13)packages/orm/src/client/contract.ts (1)
ClientContract(50-170)
packages/server/src/adapter/sveltekit/handler.ts (4)
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/types.ts (1)
Response(52-62)
packages/server/test/adapter/sveltekit.test.ts (2)
packages/testtools/src/client.ts (1)
createTestClient(52-169)packages/server/test/utils.ts (1)
makeUrl(31-33)
⏰ 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: Upload results
- GitHub Check: build-test (20.x, postgresql)
- GitHub Check: build-test (20.x, sqlite)
🔇 Additional comments (20)
packages/server/package.json (1)
102-111: LGTM! SvelteKit integration follows established adapter patterns.The export structure, dependency versions, and optional peer dependency configuration are consistent with the existing adapters in this package.
Also applies to: 124-124, 145-145, 177-179
packages/server/src/adapter/elysia/index.ts (1)
1-1: LGTM! Explicit exports improve API surface control.Replacing the wildcard export with explicit named exports is a good practice that prevents unintended public API exposure.
packages/server/src/adapter/hono/index.ts (1)
1-1: LGTM! Consistent with the adapter API surface narrowing pattern.The explicit export aligns with the refactoring across other adapters in this PR.
packages/server/src/adapter/sveltekit/handler.ts (3)
9-19: LGTM! Interface definition follows adapter patterns.The
SvelteKitHandlerOptionsinterface correctly extendsCommonAdapterOptionsand defines the requiredprefixandgetClientproperties for SvelteKit integration.
24-76: LGTM! Handler implementation follows SvelteKit patterns.The handler correctly:
- Checks for prefix-matching routes
- Retrieves the client
- Parses query and body
- Delegates to apiHandler
- Returns proper Response objects
- Falls through to
resolve(event)for non-API routes
78-78: LGTM! Export alias improves discoverability.Providing
SvelteKitHandleras an alias forcreateHandlermakes the API more intuitive and consistent with naming conventions.packages/server/src/adapter/sveltekit/index.ts (1)
1-1: LGTM! Exports align with adapter conventions.The explicit exports of
SvelteKitHandlerandSvelteKitHandlerOptionsare consistent with the patterns used in other adapters (Nuxt, Hono, etc.).packages/server/tsup.config.ts (1)
12-12: LGTM! Build entry correctly configured.The sveltekit entry is properly added to the build configuration, enabling the adapter to be bundled and exported.
packages/server/src/adapter/express/index.ts (1)
1-1: LGTM! Naming consistency improvement.Renaming to
ExpressMiddlewareOptionsmakes the type more specific and aligns with the naming conventions used across other adapters in this PR.packages/server/src/adapter/nuxt/index.ts (1)
1-1: LGTM! Explicit exports improve API clarity.The change from wildcard export to explicit named exports is consistent with the refactoring pattern applied across all adapters in this PR.
packages/server/src/adapter/fastify/index.ts (1)
1-1: LGTM! Consistent naming pattern.The type rename to
FastifyPluginOptionsimproves clarity and follows the adapter-specific naming convention used across other adapters in this PR.packages/server/test/adapter/sveltekit.test.ts (3)
1-7: LGTM! Imports are well-organized.All necessary dependencies are properly imported for testing the SvelteKit adapter with both RPC and REST handlers.
8-79: LGTM! Comprehensive RPC handler test coverage.The test suite thoroughly validates the RPC handler with:
- CRUD operations (create, findMany, update, delete)
- Nested relational writes
- Filtering and querying
- Aggregations (count, aggregate, groupBy)
All assertions properly verify status codes and response data structure.
81-136: LGTM! REST handler test coverage is solid.The test suite validates REST API operations with:
- Proper REST format (type/attributes structure)
- Query parameter filtering
- Full CRUD cycle
- Direct client verification of final state (line 134)
packages/server/src/adapter/express/middleware.ts (2)
9-9: LGTM! Type rename improves consistency.Renaming to
ExpressMiddlewareOptionsaligns with the adapter-specific naming convention applied across all adapters in this PR (Fastify, Nuxt, SvelteKit).
30-30: LGTM! Function signature updated correctly.The factory function signature properly uses the renamed
ExpressMiddlewareOptionstype, maintaining type safety.packages/server/src/adapter/fastify/plugin.ts (2)
10-10: LGTM! Type rename improves consistency.Renaming to
FastifyPluginOptionsfollows the adapter-specific naming convention and aligns with the export infastify/index.ts.
26-26: LGTM! Plugin handler signature updated correctly.The plugin callback properly uses the renamed
FastifyPluginOptions<SchemaDef>type, maintaining type safety.packages/server/src/adapter/nuxt/handler.ts (2)
17-17: LGTM! Type rename improves consistency.Renaming to
NuxtHandlerOptionsaligns with the adapter-specific naming convention used across all adapters in this PR.
24-24: LGTM! Function signature updated correctly.The
createEventHandlerfunction properly uses the renamedNuxtHandlerOptionstype, maintaining type safety.
Summary by CodeRabbit
New Features
Tests