-
Notifications
You must be signed in to change notification settings - Fork 133
chore(rivetkit): add connection for raw http #3425
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
chore(rivetkit): add connection for raw http #3425
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
3 Skipped Deployments
|
PR Review: Add connection for raw HTTPSummaryThis PR adds a new Code Quality & Best Practices✅ Good Practices:
1. Connection Lifecycle Inconsistency (rivetkit-typescript/packages/rivetkit/src/actor/router-endpoints.ts:548-572)Issue: The signature of In getConnectionReadyState: (): DriverReadyState | undefined => {
// HTTP connections are always considered open until the request completes
return DriverReadyState.OPEN;
},But the getConnectionReadyState(
actor: AnyActorInstance,
conn: AnyConn,
): DriverReadyState | undefined;Fix: Update the signature to match the interface: getConnectionReadyState: (_actor, _conn): DriverReadyState | undefined => {
return DriverReadyState.OPEN;
},This matches the pattern used in 2. Disconnect Signature Inconsistency (rivetkit-typescript/packages/rivetkit/src/actor/conn/drivers/raw-http.ts:16-18)Issue: The Current implementation: disconnect: async () => {
// Noop
},Expected signature from interface: disconnect(
actor: AnyActorInstance,
conn: AnyConn,
reason?: string,
): Promise<void>;Fix: Update to match interface: disconnect: async (_actor, _conn, _reason?) => {
// Noop
},3. Missing Error Handling Context (rivetkit-typescript/packages/rivetkit/src/actor/router-endpoints.ts:560-571)Issue: If Current code: try {
const conn = await actor.createConn(createRawHttpSocket(), {}, req);
createdConn = conn;
return await actor.handleRawRequest(req, {});
} finally {
if (createdConn) {
actor.connDisconnected(createdConn, true);
}
}Recommendation: This is actually correct as-is - if try {
const conn = await actor.createConn(createRawHttpSocket(), {}, req);
createdConn = conn;
return await actor.handleRawRequest(req, {});
} catch (error) {
// Connection creation or request handling failed
// Only clean up if connection was successfully created
throw error;
} finally {
if (createdConn) {
actor.connDisconnected(createdConn, true);
}
}Performance Considerations✅ Good:
Security ConcernsNo major security issues identified. The implementation properly:
Test Coverage
Recommendation: Add tests to verify:
Additional NotesRouter path handling (rivetkit-typescript/packages/rivetkit/src/actor/router.ts:178): const originalPath = url.pathname.replace(/^\/request/, "") || "/";This is simpler than the previous implementation and correctly handles the path stripping. SummaryRequired Changes:
Recommended Changes:
Overall, this is a solid implementation that follows existing patterns well. The required fixes are minor TypeScript signature issues that should be straightforward to address. |
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: |
2c681e1 to
d1c3287
Compare
5bf1455 to
fc159fd
Compare
Code Review: PR #3425 - Add Connection for Raw HTTPOverviewThis PR adds a new ✅ Positive Observations
🔍 Issues & Concerns1. Missing Connection Cleanup on Error Path
|
PR Review: Add Connection for Raw HTTPSummaryThis PR adds connection lifecycle management for raw HTTP requests by creating a Code Quality & Best Practices ✅Positive aspects:
Issues & Concerns1. Missing connection cleanup on error path
|
fc159fd to
1109cc5
Compare
1109cc5 to
b3ff582
Compare
d1c3287 to
7095d1c
Compare
|
Deployment failed with the following error: Learn More: https://vercel.com/docs/limits#rate-limits |
Code Review: Raw HTTP Connection DriverThank you for this contribution! This PR adds connection lifecycle management for raw HTTP requests, bringing consistency with how other connection types (WebSocket, HTTP actions) are handled. Overall, the implementation is solid and follows existing patterns. Here are my findings: ✅ Strengths
🐛 Potential IssuesCritical: Missing Parameters in
|
Merge activity
|

No description provided.