refactor(rivetkit): extract WebSocket protocol parsing to shared utility #3429
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
3 Skipped Deployments
|
Code Review for PR #3429SummaryThis PR successfully refactors WebSocket protocol parsing logic by extracting duplicated code into a shared utility function ✅ Positive Changes
🔍 Code Quality & Best PracticesIssue 1: Silent fallback behavior change in The PR changes // Before:
if (!encodingParam) {
throw new errors.InvalidEncoding("undefined");
}
// After:
if (!encodingParam) {
return "json";
}Concerns:
Recommendation: Consider one of these approaches:
Issue 2: Missing error handling in const encoding = EncodingSchema.parse(encodingRaw);
const connParams = connParamsRaw ? JSON.parse(connParamsRaw) : undefined;Both
Recommendation:
Example: const result = EncodingSchema.safeParse(encodingRaw);
if (!result.success) {
throw new errors.InvalidEncoding(encodingRaw);
}
const encoding = result.data;
let connParams: unknown;
if (connParamsRaw) {
try {
connParams = JSON.parse(connParamsRaw);
} catch (error) {
throw new errors.MalformedMessage(`Invalid connection parameters: ${error}`);
}
}🐛 Potential BugsBug 1: If no Expected behavior: Should either:
Bug 2: Inconsistent default behavior
This creates inconsistent behavior between HTTP requests and WebSocket connections. 🔒 Security ConcernsLow-severity concern: Missing URL decode validation connParamsRaw = decodeURIComponent(
protocol.substring(WS_PROTOCOL_CONN_PARAMS.length),
);While ⚡ Performance ConsiderationsNo significant performance concerns. The refactoring actually improves performance slightly by:
🧪 Test CoverageMissing test coverage:
Recommendation:
📋 Suggested Changes
Summary Rating
Overall, this is a good refactoring that improves code maintainability, but it needs stronger error handling and test coverage before merging. |
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 Review for PR #3429SummaryThis PR refactors WebSocket protocol parsing logic by extracting duplicated code into a shared utility function Positive AspectsCode Quality:
Behavior Change:
Issues & Concerns🔴 Critical: Missing Error HandlingLocation: The new const encoding = EncodingSchema.parse(encodingRaw);
const connParams = connParamsRaw ? JSON.parse(connParamsRaw) : undefined;Problems:
Impact:
Previous Behavior: Recommended Fix: export function parseWebSocketProtocols(protocols: string | null | undefined): {
encoding: Encoding;
connParams: unknown;
} {
let encodingRaw: string | undefined;
let connParamsRaw: string | undefined;
if (protocols) {
const protocolList = protocols.split(",").map((p) => p.trim());
for (const protocol of protocolList) {
if (protocol.startsWith(WS_PROTOCOL_ENCODING)) {
encodingRaw = protocol.substring(WS_PROTOCOL_ENCODING.length);
} else if (protocol.startsWith(WS_PROTOCOL_CONN_PARAMS)) {
connParamsRaw = decodeURIComponent(
protocol.substring(WS_PROTOCOL_CONN_PARAMS.length),
);
}
}
}
// Handle encoding with proper error handling
const encodingResult = EncodingSchema.safeParse(encodingRaw);
if (!encodingResult.success) {
throw new errors.InvalidEncoding(encodingRaw);
}
const encoding = encodingResult.data;
// Handle conn params with proper error handling
let connParams: unknown = undefined;
if (connParamsRaw) {
try {
connParams = JSON.parse(connParamsRaw);
} catch (err) {
throw new errors.InvalidParams(
`Invalid connection params JSON: ${err}`,
);
}
}
return { encoding, connParams };
}This would:
🟡 Minor: Inconsistent Default BehaviorLocation: The
This inconsistency could confuse users. Consider applying the same default behavior: const encodingResult = EncodingSchema.safeParse(encodingRaw ?? "json");Security Considerations✅ No security issues identified. The refactoring maintains the same security properties as the original code:
Performance Considerations✅ Performance is neutral or slightly improved:
Test Coverage
Example test cases to consider: // Valid cases
parseWebSocketProtocols("rivet_encoding.json")
parseWebSocketProtocols("rivet_encoding.cbor,rivet_conn_params.%7B%22foo%22%3A%22bar%22%7D")
// Error cases (once error handling is added)
parseWebSocketProtocols("") // Should use default or error
parseWebSocketProtocols("rivet_encoding.invalid") // Invalid encoding
parseWebSocketProtocols("rivet_conn_params.{invalid-json}") // Malformed JSONRecommendationsBefore Merging:
Future Improvements:
ConclusionThis is a solid refactoring that improves code maintainability. However, the missing error handling in The refactoring itself is well-structured and follows good practices—it just needs better error handling to match the quality of the rest of the codebase. |
NathanFlurry
force-pushed
the
11-09-refactor_rivetkit_restructure_connection_lifecycle_contexts
branch
from
7de7488 to
27c352a
Compare
NathanFlurry
force-pushed
the
11-09-refactor_rivetkit_extract_websocket_protocol_parsing_to_shared_utility
branch
from
8fdd032 to
14348ed
Compare
|
Deployment failed with the following error: Learn More: https://vercel.com/docs/limits#rate-limits |
PR Review: Extract WebSocket Protocol Parsing to Shared UtilitySummaryThis PR successfully extracts duplicated WebSocket protocol parsing logic into a shared utility function ( Positive AspectsCode Quality ✅
Improved User Experience ✅
Issues Found1. Critical: Inconsistent Error Handling 🔴The new Before (router.ts:142): const encoding = EncodingSchema.parse(encodingRaw);After (router-endpoints.ts:600): const encoding = EncodingSchema.parse(encodingRaw);The issue is that when Recommendation: Add a default value or handle the undefined case: const encoding = EncodingSchema.parse(encodingRaw ?? "json");This would align with the new behavior of 2. Bug: JSON.parse Can Throw 🟡In const connParams = connParamsRaw ? JSON.parse(connParamsRaw) : undefined;The Recommendation: Wrap in try-catch and throw an appropriate let connParams: unknown;
if (connParamsRaw) {
try {
connParams = JSON.parse(connParamsRaw);
} catch (error) {
throw new errors.InvalidParams(`Failed to parse connection parameters: ${error}`);
}
}3. Inconsistency: getRequestEncoding Change 🟡The change to
This behavioral difference between the two protocols is confusing and could lead to bugs. Recommendation: Apply the same default behavior to both protocols for consistency. Performance Considerations✅ No performance concerns: The refactoring doesn't introduce any performance regressions. String operations are minimal and well-contained. Security Considerations
Test Coverage
Recommendations:
Additional ObservationsCode Style ✅
DocumentationThe added comment about defaulting to JSON for curl requests is helpful. Consider adding similar documentation to SummaryOverall assessment: Good refactoring with some critical issues to address Required fixes:
Nice to have:
Once these issues are addressed, this will be a solid improvement to the codebase. 🤖 Generated with Claude Code |
NathanFlurry
force-pushed
the
11-09-refactor_rivetkit_extract_websocket_protocol_parsing_to_shared_utility
branch
from
14348ed to
e10206c
Compare
Code Review: refactor(rivetkit): extract WebSocket protocol parsing to shared utilitySummaryThis PR extracts duplicated WebSocket protocol parsing logic into a shared utility function Positive Aspects ✅
Issues & Concerns 🔴1. Critical: Error Handling InconsistencyThe new Before (original code): // Both locations handled the case where encoding is undefined
const encoding = EncodingSchema.parse(encodingRaw);After (new function): // Line 600 in router-endpoints.ts
const encoding = EncodingSchema.parse(encodingRaw);Problem: When Original behavior in
New behavior in
2. Potential Bug: JSON Parsing ExceptionLine 601: const connParams = connParamsRaw ? JSON.parse(connParamsRaw) : undefined;Issue: Impact: An attacker or buggy client could send malformed JSON in the connection parameters, causing an unhandled exception that crashes the WebSocket connection handler. 3. Type Safety: Missing Return Type AnnotationsThe function returns an inline object type. Consider defining a proper type: export interface WebSocketProtocolConfig {
encoding: Encoding;
connParams: unknown;
}
export function parseWebSocketProtocols(
protocols: string | null | undefined
): WebSocketProtocolConfig {
// ...
}Recommendations 🔧High Priority:
export function parseWebSocketProtocols(protocols: string | null | undefined): {
encoding: Encoding;
connParams: unknown;
} {
let encodingRaw: string | undefined;
let connParamsRaw: string | undefined;
if (protocols) {
const protocolList = protocols.split(",").map((p) => p.trim());
for (const protocol of protocolList) {
if (protocol.startsWith(WS_PROTOCOL_ENCODING)) {
encodingRaw = protocol.substring(WS_PROTOCOL_ENCODING.length);
} else if (protocol.startsWith(WS_PROTOCOL_CONN_PARAMS)) {
connParamsRaw = decodeURIComponent(
protocol.substring(WS_PROTOCOL_CONN_PARAMS.length),
);
}
}
}
// Use safeParse like getRequestEncoding does
const result = EncodingSchema.safeParse(encodingRaw);
if (\!result.success) {
throw new errors.InvalidEncoding(encodingRaw || "undefined");
}
// Add error handling for JSON parsing
let connParams: unknown = undefined;
if (connParamsRaw) {
try {
connParams = JSON.parse(connParamsRaw);
} catch (err) {
throw new errors.InvalidParams(
`Invalid conn params JSON: ${err}`,
);
}
}
return { encoding: result.data, connParams };
}
Medium Priority:
Security Considerations 🔒
Test Coverage 📊Current: No tests found for this new function Overall AssessmentThis is a good refactoring that improves code organization, but it has critical error handling regressions that need to be addressed before merging. The extracted function is less robust than the code it's replacing, which is a step backward in terms of error handling and user experience. Recommendation: Request changes to fix error handling and add tests. Files reviewed:
|
Merge activity
|
graphite-app
bot
deleted the
11-09-refactor_rivetkit_extract_websocket_protocol_parsing_to_shared_utility
branch
No description provided.