-
Notifications
You must be signed in to change notification settings - Fork 131
chore(rivetkit): add support for conn params on onRequest and onWebSocket #3445
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
base: 11-10-feat_rivetkit_add_asyncapi_spec
Are you sure you want to change the base?
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
3 Skipped Deployments
|
Code ReviewSummaryThis PR adds support for passing connection parameters ( Positive Aspects ✅
Issues & Concerns 🔍1. Type Safety - CriticalThe connParams: unknown | undefined,However, when calling Recommendation: Consider validating/parsing 2. Null vs Undefined Inconsistency - MediumIn const parameters = getRequestConnParams(c.req); // Could be null
// ...
parameters, // Passed directly to prepareAndConnectConnMeanwhile, for WebSocket you handle undefined with Recommendation: Make the return types consistent - either use 3. Missing Comment Removal - MinorIn router.ts:113, a comment was removed: -// Parse configuration from Sec-WebSocket-Protocol header
const protocols = c.req.header("sec-websocket-protocol");While the comment might have been redundant, it is worth noting this was the only comment removal in the PR. Not a functional issue, but worth confirming it was intentional. Testing Concerns 🧪Missing Test Coverage: There do not appear to be tests added for this new functionality. Consider adding tests for:
Given that this touches connection management which is critical infrastructure, test coverage would be valuable. Performance Considerations ⚡No performance concerns identified. The changes are minimal and follow existing patterns. Security Considerations 🔒Input Validation: The
This is reasonable, but consider:
Recommendations Summary
ConclusionThe implementation is solid and follows existing patterns well. The main concerns are around type safety and test coverage. With the recommendations addressed, this would be ready to merge. 🤖 Generated with Claude Code |
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: |

No description provided.