feat(server): migrate next.js server adapter #335
Conversation
|
Caution Review failedThe pull request is closed. 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. WalkthroughReorganizes server adapters: moves Express adapter into Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–30 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
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 Pull Request ReviewI've completed a comprehensive review of the Next.js server adapter migration. Here's my detailed analysis:
Code Quality & ArchitectureExcellent Implementation ✅
Architecture Highlights:
Security AnalysisGood Security Practices ✅
Minor Security Considerations:
Test CoverageComprehensive Testing ✅
Missing Test Areas:
Performance ConsiderationsEfficient Implementation ✅
Performance Notes:
Integration & ConsistencyWell Integrated ✅
Issues FoundCritical Issue
Suggested Fix: "./next": {
"import": {
"types": "./dist/next.d.ts",
"default": "./dist/next.js"
},
"require": {
"types": "./dist/next.d.cts",
"default": "./dist/next.cjs"
}
}Minor Issues:
Recommendations
Overall, this is a high-quality implementation that follows ZenStack patterns well. The critical export issue needs to be addressed before merge. |
There was a problem hiding this comment.
Pull Request Overview
This PR migrates the Next.js server adapter into a new unified adapter structure alongside the Express adapter. The changes reorganize the codebase to support multiple framework adapters under a common adapter/ directory pattern.
Key changes:
- Restructured adapter organization with Express and Next.js adapters under
src/adapter/ - Added comprehensive test coverage for the Next.js adapter supporting both Pages Router and App Router
- Updated build configuration and dependencies to include Next.js as an optional peer dependency
Reviewed Changes
Copilot reviewed 8 out of 10 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/server/tsup.config.ts | Updated build entry points to reflect new adapter directory structure |
| packages/server/test/adapter/next.test.ts | Added comprehensive test suite for Next.js adapter with RPC handler tests |
| packages/server/test/adapter/express.test.ts | Updated import path to reference new adapter directory structure |
| packages/server/src/adapter/next/pages-route-handler.ts | Implemented Pages Router request handler for Next.js API routes |
| packages/server/src/adapter/next/index.ts | Created main entry point with type definitions and factory function for Next.js adapters |
| packages/server/src/adapter/next/app-route-handler.ts | Implemented App Router request handler for Next.js 13+ route handlers |
| packages/server/src/adapter/express/middleware.ts | Updated import path and added missing JSDoc comment for apiHandler option |
| packages/server/package.json | Added Next.js as optional peer dependency and dev dependency |
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
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: Missing export entry for the Next.js adapter.The build configuration (tsup.config.ts) generates a
nextentry, butpackage.jsondoesn't expose it in the exports field. Users won't be able to import the Next.js adapter.Add the Next.js export entry:
"./express": { "import": { "types": "./dist/express.d.ts", "default": "./dist/express.js" }, "require": { "types": "./dist/express.d.cts", "default": "./dist/express.cjs" } + }, + "./next": { + "import": { + "types": "./dist/next.d.ts", + "default": "./dist/next.js" + }, + "require": { + "types": "./dist/next.d.cts", + "default": "./dist/next.cjs" + } }
🧹 Nitpick comments (3)
packages/server/src/adapter/next/pages-route-handler.ts (2)
29-29: Handle potentially undefined request method.While Next.js typically populates
req.method, the non-null assertion operator could mask edge cases. Consider explicit handling or validation.Apply this diff to handle undefined method:
- const r = await options.apiHandler.handleRequest({ - method: req.method!, + if (!req.method) { + res.status(400).json({ message: 'missing request method' }); + return; + } + const r = await options.apiHandler.handleRequest({ + method: req.method,
35-35: Use consistent response method.Error responses use
.json()(lines 17, 22, 37), but the success response uses.send(). While both work, consistency improves maintainability.Apply this diff:
- res.status(r.status).send(r.body); + res.status(r.status).json(r.body);packages/server/src/adapter/next/app-route-handler.ts (1)
53-53: Handle potentially undefined request method.While Next.js typically populates
req.method, the non-null assertion operator could mask edge cases. Consider explicit handling or validation.Apply this diff:
+ if (!req.method) { + return NextResponse.json({ message: 'missing request method' }, { status: 400 }); + } + try { const r = await options.apiHandler.handleRequest({ - method: req.method!, + method: req.method,
📜 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 (8)
packages/server/package.json(1 hunks)packages/server/src/adapter/express/middleware.ts(1 hunks)packages/server/src/adapter/next/app-route-handler.ts(1 hunks)packages/server/src/adapter/next/index.ts(1 hunks)packages/server/src/adapter/next/pages-route-handler.ts(1 hunks)packages/server/test/adapter/express.test.ts(1 hunks)packages/server/test/adapter/next.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/middleware.tspackages/server/tsup.config.tspackages/server/test/adapter/express.test.tspackages/server/src/adapter/next/index.tspackages/server/src/adapter/next/pages-route-handler.tspackages/server/package.jsonpackages/server/test/adapter/next.test.tspackages/server/src/adapter/next/app-route-handler.ts
🧬 Code graph analysis (5)
packages/server/src/adapter/express/middleware.ts (1)
packages/sdk/src/schema/schema.ts (1)
SchemaDef(10-18)
packages/server/src/adapter/next/index.ts (3)
packages/sdk/src/schema/schema.ts (1)
SchemaDef(10-18)packages/server/src/types.ts (1)
ApiHandler(67-77)packages/orm/src/client/contract.ts (1)
ClientContract(50-170)
packages/server/src/adapter/next/pages-route-handler.ts (3)
packages/server/src/adapter/next/app-route-handler.ts (1)
factory(13-64)packages/sdk/src/schema/schema.ts (1)
SchemaDef(10-18)packages/server/src/adapter/next/index.ts (1)
PageRouteRequestHandlerOptions(19-29)
packages/server/test/adapter/next.test.ts (3)
packages/server/src/adapter/next/index.ts (2)
PageRouteRequestHandlerOptions(19-29)NextRequestHandler(55-63)packages/sdk/src/schema/schema.ts (1)
SchemaDef(10-18)packages/testtools/src/client.ts (2)
createTestClient(52-169)createPolicyTestClient(179-190)
packages/server/src/adapter/next/app-route-handler.ts (3)
packages/server/src/adapter/next/pages-route-handler.ts (1)
factory(11-40)packages/sdk/src/schema/schema.ts (1)
SchemaDef(10-18)packages/server/src/adapter/next/index.ts (1)
AppRouteRequestHandlerOptions(34-44)
🔇 Additional comments (8)
packages/server/tsup.config.ts (1)
6-7: LGTM!The build configuration correctly reflects the new adapter structure with both Express and Next.js entries.
packages/server/src/adapter/express/middleware.ts (2)
4-4: LGTM!The import path correctly reflects the new adapter directory structure.
10-12: LGTM!The added documentation improves API clarity.
packages/server/test/adapter/express.test.ts (1)
6-6: LGTM!The import path correctly reflects the new adapter directory structure.
packages/server/test/adapter/next.test.ts (1)
10-43: LGTM!The test helper correctly simulates Next.js API routing and integrates with the apiResolver for end-to-end-style testing.
packages/server/src/adapter/next/index.ts (3)
9-14: LGTM!The common options interface provides clean abstraction for shared adapter configuration.
19-29: LGTM!The Pages Router options correctly extend common options with page-specific getClient signature.
49-63: LGTM!The overloaded function signatures provide excellent type safety, distinguishing between Pages Router and App Router based on the
useAppDirflag.
|
Claude finished @ymc9's task —— View job Pull Request ReviewI've completed a comprehensive review of the Next.js server adapter migration. Here's my detailed analysis: Todo List:
Code Quality & ArchitectureExcellent Implementation ✅
Architecture Highlights:
Security AnalysisGood Security Practices ✅
Minor Security Considerations:
Test CoverageComprehensive Testing ✅
Missing Test Areas:
Performance ConsiderationsEfficient Implementation ✅
Performance Notes:
Integration & ConsistencyWell Integrated ✅
Issues FoundCritical Issue
Suggested Fix: "./next": {
"import": {
"types": "./dist/next.d.ts",
"default": "./dist/next.js"
},
"require": {
"types": "./dist/next.d.cts",
"default": "./dist/next.cjs"
}
}Minor Issues:
Recommendations
Overall, this is a high-quality implementation that follows ZenStack patterns well. The critical export issue needs to be addressed before merge. |
Summary by CodeRabbit
New Features
Chores
Tests