-
Notifications
You must be signed in to change notification settings - Fork 129
fix(rivetkit): fix missing duplex in nextjs in development #3388
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
|
The latest updates on your projects. Learn more about Vercel for GitHub.
3 Skipped Deployments
|
72433e4 to
91efc39
Compare
b760394 to
64aafa4
Compare
Code Review for PR #3388SummaryThis PR adds the ✅ Positive Aspects
🔍 Observations & Suggestions1. Consistency with Production PathThe production path (lines 59-61) creates a Request differently: const newReq = new Request(newUrl, request);While the development path explicitly copies all properties. Consider whether the production path might need the same fix if it encounters requests with bodies. The simplified constructor Recommendation: Test or document whether the production path handles POST/PUT/PATCH requests with bodies correctly, or if it needs the same explicit duplex configuration. 2. TypeScript Type CastThe Recommendation: Consider adding a comment explaining why the cast is necessary (TypeScript types may lag behind the spec) to help future maintainers. 3. Missing Test CoverageNo test files were found for this package. The change affects request handling with bodies, which should ideally have test coverage. Recommendation: Consider adding integration tests that verify:
4. Error HandlingThe current implementation doesn't validate whether Recommendation: Consider defensive checks, though the current implementation is likely fine for the intended use case. 🎯 Code Quality Assessment
Final VerdictLGTM with minor suggestions. This is a solid fix for a real runtime issue. The main improvement would be ensuring test coverage for request handling with bodies, and potentially verifying the production code path handles this correctly as well. The change is safe to merge as-is, with the suggestions being opportunities for future enhancement rather than blockers. |
PR Review: Fix missing duplex in Next.js developmentSummaryThis PR fixes a runtime error in Next.js development mode by adding the required Code Quality ✅Positive aspects:
Consistency: This pattern is already used in 7+ other locations in the rivetkit-typescript codebase, making this change consistent with established practices. Potential Issues
|
More templates
@rivetkit/actor
@rivetkit/cloudflare-workers
@rivetkit/core
@rivetkit/db
@rivetkit/framework-base
@rivetkit/next-js
@rivetkit/react
rivetkit
@rivetkit/sql-loader
@rivetkit/engine-runner
@rivetkit/engine-runner-protocol
commit: |
Code ReviewSummaryThis PR fixes a Positive Observations✅ Correct Fix: The change addresses a legitimate issue with the Fetch API specification. When creating a Request with a streaming body, the ✅ Consistent with Codebase: I found 7+ other instances throughout the codebase where
✅ Proper Type Casting: Using ✅ Good Comment: The inline comment explains why this property is needed. Potential Issues & Suggestions
if (mergedInit.body) {
(mergedInit as any).duplex = "half";
}In const newReq = new Request(newUrl, {
method: request.method,
headers: request.headers,
body: request.body,
credentials: request.credentials,
cache: request.cache,
redirect: request.redirect,
referrer: request.referrer,
integrity: request.integrity,
signal: mergedController.signal,
...(request.body && { duplex: "half" }),
} as RequestInit);However, this is a minor point and the current implementation is acceptable.
Security & Performance✅ No Security Concerns: The change is purely fixing a spec compliance issue. ✅ No Performance Impact: Adding a property to the RequestInit object has negligible performance impact. VerdictLGTM 👍 This is a valid bug fix that aligns with the Fetch API specification and follows patterns already established in the codebase. The only minor suggestion is to consider conditional application of the Review performed by Claude Code - #3388 |
64aafa4 to
b515fae
Compare
91efc39 to
b80bfa4
Compare
PR Review: Fix missing duplex in Next.js developmentSummaryThis PR adds the Code Quality ✅Positive:
Observations:
Potential Issues 🔍Minor considerations:
Testing Considerations 🧪
Performance ✅
Security ✅
Recommendations
Verdict ✅LGTM - This is a solid fix for the development environment issue. The only consideration is whether the production path needs the same treatment, but that can be addressed in a follow-up if needed. Review generated with Claude Code |
Pull Request ReviewSummaryThis PR fixes a ✅ Positive Findings1. Correct Fix 2. Consistent with Codebase Patterns
3. Appropriate TypeScript Cast 4. Good Code Comment 🤔 Observations & Suggestions1. Inconsistency with Production Code Path const newReq = new Request(newUrl, request);This uses the Request copy constructor, which should automatically handle the duplex property. However, the development path manually spreads all properties and rebuilds the Request, which is why the duplex property needs to be explicitly set. Question: Could the development path also use the Request copy constructor approach and only override the signal? For example: const newReq = new Request(newUrl, {
...request,
signal: mergedController.signal,
});This might avoid the need for manual duplex handling, though I understand there may be specific reasons for the current approach. 2. Conditional Duplex (Minor) if (mergedInit.body) {
(mergedInit as any).duplex = "half";
}While the current fix always adds duplex, this appears safe since having it when there's no body shouldn't cause issues. However, for consistency with other parts of the codebase, you could consider: // Required for streaming body
...(request.body && { duplex: "half" }),That said, this is a minor point and the current approach is perfectly valid. 🧪 Test CoverageObservation: No test files exist for this package ( Recommendation: While this fix is straightforward, consider adding integration tests that:
This would prevent regressions and make the package more robust. 🔒 Security & PerformanceSecurity: ✅ No security concerns. The change only affects request creation mechanics. Performance: ✅ No performance impact. Adding a property to an options object has negligible overhead. Final Verdict✅ LGTM (Looks Good To Me) This is a solid bug fix that:
The suggestions above are minor improvements for consideration, not blockers. The PR is ready to merge as-is. Recommendation: Approve and merge. |
b515fae to
c4582a0
Compare
b80bfa4 to
7091bb6
Compare
Code Review - PR #3388SummaryThis PR fixes a runtime error in Next.js development mode by adding the required ✅ Positive Findings1. Correct Fix 2. Consistent with Codebase Patterns
All these files cast to 3. Appropriate Use of Type Assertion 4. Good Comment 🔍 Observations1. Scope is Correct
2. Request Body Handling
🤔 Minor Considerations1. Conditional Application if (mergedInit.body) {
(mergedInit as any).duplex = "half";
}However, in this case, always adding
2. Test Coverage ✅ Final VerdictApprove - This is a clean, correct fix that:
RecommendationMerge - No changes needed. |
Merge activity
|
``` RequestInit: duplex option is required when sending a body. ```

Uh oh!
There was an error while loading. Please reload this page.