-
Notifications
You must be signed in to change notification settings - Fork 133
chore(rivetkit): add WebSocketContext and RequestContext
#3427
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 WebSocketContext and RequestContext
#3427
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
3 Skipped Deployments
|
PR Review: Add
|
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: |
b1ac4de to
9878acc
Compare
22fd1d2 to
7ac46b6
Compare
Code ReviewThis PR introduces specialized context types ( Positives1. Strong API Design
2. Code Quality
3. Excellent Documentation
4. Proper Export Management
Suggestions1. Missing Documentation on /**
* Should not be called directly.
*
* @param actorContext - The actor context
* @param conn - The connection associated with the request
*/
constructor(...)Consider adding: /**
* Creates a new RequestContext. Should not be called directly by user code.
* This is automatically created by the RivetKit runtime when handling raw HTTP requests.
*
* @internal
* @param actorContext - The underlying actor context providing access to actor-level operations
* @param conn - The connection associated with this specific HTTP request
*/
constructor(...)The 2. Consider Readonly State Access get state(): Readonly<TState> {
return this.#actorContext.state;
}This is a design question rather than a bug - just worth considering the intended behavior. 3. Type Safety Enhancement (Optional) broadcast(name: string, ...args: any[]): voidConsider using broadcast(name: string, ...args: unknown[]): voidSecurity & Performance
Test CoverageThe fixture updates in Consider adding explicit tests for:
Overall AssessmentThis is a high-quality PR that improves the API design without breaking changes beyond the context type. The implementation is consistent with existing patterns, well-documented, and type-safe. The suggestions above are minor improvements rather than blockers. Recommendation: Approve with optional improvements Great work on maintaining architectural consistency! |
Pull Request Review: Add
|
9878acc to
57377f5
Compare
7ac46b6 to
1f30194
Compare
|
Deployment failed with the following error: Learn More: https://vercel.com/docs/limits#rate-limits |
Pull Request ReviewThis PR introduces Strengths
Issues Found1. Code Duplication (Medium Priority)Location: Both files are nearly identical (183 lines each, identical implementation). This violates DRY principles and creates maintenance burden. Suggestion: Consider creating a base class or using a generic factory function: // Option 1: Base class
abstract class ConnContext<TState, TConnParams, TConnState, TVars, TInput, TDatabase extends AnyDatabaseProvider> {
#actorContext: ActorContext<...>;
public readonly conn: Conn<...>;
constructor(actorContext, conn) { ... }
// All shared getters and methods
}
export class RequestContext<...> extends ConnContext<...> {}
export class WebSocketContext<...> extends ConnContext<...> {}2. Inconsistent Parameter Removal (Low Priority)Location: The
The empty
Question: Is there a reason for this inconsistency? If 3. Typo in Error Class (Low Priority)Location: throw new errors.RequestHandlerNotDfeined();
// ^ Should be "Defined"This typo exists in the current code and is not introduced by this PR, but it would be good to fix it while you're working in this area. Observations
Test CoverageThe existing test suite appears comprehensive:
Suggestion: Consider adding a test that explicitly verifies the test("should provide connection in RequestContext", async (c) => {
const actor = client.someActor.getOrCreate(["test"]);
const response = await actor.fetch("test/conn");
const data = await response.json();
// Verify conn properties are accessible
expect(data.hasConn).toBe(true);
expect(data.connId).toBeTruthy();
});Performance ConsiderationsNo performance concerns. The wrapper contexts add minimal overhead (one additional object allocation per request/WebSocket). Security ConsiderationsNo security concerns identified. The contexts properly encapsulate the connection and delegate to the actor context without exposing internals. Recommendations
SummaryThis is a solid PR that improves the RivetKit API. The main concern is code duplication, which should be addressed before merging. The breaking change is justified by the improved API clarity. Overall Assessment: Approve with recommendations for improvements. |
PR Review: Add
|
Merge activity
|

No description provided.