Skip to content

Conversation

@NathanFlurry
Copy link
Member

No description provided.

@vercel
Copy link

vercel bot commented Nov 10, 2025

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Preview Comments Updated (UTC)
rivetkit-serverless Ready Ready Preview Comment Nov 12, 2025 5:07pm
3 Skipped Deployments
Project Deployment Preview Comments Updated (UTC)
rivet-cloud Ignored Ignored Nov 12, 2025 5:07pm
rivet-inspector Ignored Ignored Preview Nov 12, 2025 5:07pm
rivet-site Ignored Ignored Preview Nov 12, 2025 5:07pm

Copy link
Member Author

NathanFlurry commented Nov 10, 2025

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more


How to use the Graphite Merge Queue

Add the label merge-queue to this PR to add it to the merge queue.

You must have a Graphite account in order to use the merge queue. Sign up using this link.

An organization admin has enabled the Graphite Merge Queue in this repository.

Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue.

This stack of pull requests is managed by Graphite. Learn more about stacking.

@claude
Copy link

claude bot commented Nov 10, 2025

PR Review: Add WebSocketContext and RequestContext

Overview

This PR introduces specialized context classes (RequestContext and WebSocketContext) for raw HTTP and WebSocket handlers, providing better API ergonomics and clearer separation of concerns. The changes improve type safety by exposing the conn object directly while maintaining access to actor-level functionality.


✅ Positives

  1. Improved API Design: The new context classes provide a cleaner, more intuitive interface for handler functions. Separating request/WebSocket contexts from the general ActorContext makes the API more discoverable and easier to use.

  2. Better Type Safety: By having dedicated context types, developers get better IDE autocomplete and type checking for request vs WebSocket handlers.

  3. Consistent Pattern: The implementation follows the existing ActionContext and ActorContext pattern, maintaining consistency across the codebase.

  4. Good Documentation: All new classes and methods have proper JSDoc comments with type parameter descriptions.

  5. Test Coverage: The changes update existing test fixtures, ensuring the new API works correctly.

  6. Proper Encapsulation: Uses private fields (#actorContext) and delegates to the underlying ActorContext, following good OOP principles.


🔍 Code Quality Observations

Good Practices:

  • Hard tabs are used consistently (per CLAUDE.md conventions)
  • Proper use of TypeScript's private fields syntax
  • Type-only imports where appropriate
  • Exports properly added to the module index

Architecture:

  • The delegation pattern to ActorContext is clean and maintainable
  • Connection is now exposed directly via public readonly conn, which improves usability

🤔 Suggestions & Considerations

1. Code Duplication Between Context Classes

The RequestContext and WebSocketContext classes are nearly identical (183 lines each), with only the class name and documentation comments differing. This creates maintenance overhead.

Consideration:

  • Could these share a common base class or use composition to reduce duplication?
  • If they're intentionally kept separate for future divergence, this is acceptable but should be documented.

Example approach (if desired):

abstract class BaseHandlerContext<...> {
  #actorContext: ActorContext<...>;
  public readonly conn: Conn<...>;
  
  // All shared getters and methods here...
}

export class RequestContext<...> extends BaseHandlerContext<...> {
  // Request-specific methods (if any in the future)
}

export class WebSocketContext<...> extends BaseHandlerContext<...> {
  // WebSocket-specific methods (if any in the future)
}

However, if the plan is to add specialized methods to each context type in the future, the current approach is fine.

2. Missing Return Value in config.ts

In rivetkit-typescript/packages/rivetkit/src/actor/config.ts:431, the signature was updated but there's a slight inconsistency:

onRequest?: (
  c: RequestContext<...>,
  request: Request,
) => Response | Promise<Response>;

The return type looks correct, but the implementation in instance/mod.ts:661 checks:

if (!response) {
  throw new errors.InvalidRequestHandlerResponse();
}

This suggests Response cannot be nullable, which is correct. However, the JSDoc says:

@returns A Response object to send back, or void to continue with default routing

This comment appears to be outdated - the type signature doesn't allow void. Should either:

  • Update the JSDoc to remove "or void to continue with default routing"
  • OR update the type to allow void | undefined if that's intentional

3. Removed opts Parameter

The onRequest handler previously had an empty opts: {} parameter that was removed. While this simplifies the API now, if you anticipate needing options in the future, you might want to keep it (even empty) for API stability. However, removing it is cleaner if options aren't needed.

Similarly, onWebSocket now has opts: { request: Request } which is good - it provides the upgrade request context.

4. Missing Tests for New Context Classes

While the test fixtures were updated to use the new types, I don't see explicit tests for:

  • Verifying that RequestContext.conn is properly set
  • Verifying that WebSocketContext.conn is properly set
  • Testing that all delegated methods work correctly

Recommendation: Add integration tests that verify:

// In a test actor
onRequest(ctx: RequestContext<...>, request: Request) {
  assert(ctx.conn !== undefined);
  assert(ctx.conn.id === expectedConnId);
  // Test other context methods...
  return new Response("ok");
}

🔒 Security Considerations

No security concerns identified. The changes maintain the same access control patterns as before.


⚡ Performance Considerations

Minor Overhead: Creating an additional context wrapper object on each request/WebSocket connection adds minimal overhead. Given that these are relatively infrequent operations (compared to action calls), this is negligible.

Optimization Note: The contexts delegate all calls to ActorContext, which in turn delegates to ActorInstance. This is an acceptable pattern, but be aware of the indirection chain for any future performance-sensitive paths.


📋 Breaking Changes

This is a breaking change for any code using onRequest or onWebSocket handlers:

  • Parameter signature changed from ActorContext to RequestContext/WebSocketContext
  • The opts parameter was removed from onRequest

Migration path is straightforward:

// Before
onRequest(ctx: ActorContext<...>, request: Request, opts: {}) {
  // No access to conn
}

// After
onRequest(ctx: RequestContext<...>, request: Request) {
  // Can now access ctx.conn
}

✅ Final Verdict

Approve with minor suggestions. The changes are well-structured and improve the API significantly. The main areas for consideration are:

  1. Documentation fix for the JSDoc return type comment in config.ts:416
  2. Consider addressing code duplication between context classes (not blocking)
  3. Consider adding explicit tests for the new context types (not blocking)

The breaking changes are justified by the improved API, and the migration path is clear.


Overall: Strong improvement to the RivetKit API. Nice work! 🚀

@pkg-pr-new
Copy link

pkg-pr-new bot commented Nov 10, 2025

More templates

@rivetkit/actor

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/actor@3427

@rivetkit/cloudflare-workers

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/cloudflare-workers@3427

@rivetkit/core

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/core@3427

@rivetkit/db

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/db@3427

@rivetkit/framework-base

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/framework-base@3427

@rivetkit/next-js

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/next-js@3427

@rivetkit/react

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/react@3427

rivetkit

pnpm add https://pkg.pr.new/rivet-dev/rivet/rivetkit@3427

@rivetkit/sql-loader

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/sql-loader@3427

@rivetkit/engine-runner

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/engine-runner@3427

@rivetkit/engine-runner-protocol

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/engine-runner-protocol@3427

commit: af173a9

@NathanFlurry NathanFlurry changed the base branch from 11-09-chore_rivetkit_add_conn_type to graphite-base/3427 November 11, 2025 00:07
@NathanFlurry NathanFlurry changed the base branch from graphite-base/3427 to 11-09-chore_rivetkit_add_conn_type November 11, 2025 00:08
@claude
Copy link

claude bot commented Nov 12, 2025

Code Review: Add WebSocketContext and RequestContext

Thanks for this refactor! Overall this is a solid improvement that provides better type safety and clearer separation of concerns for raw HTTP and WebSocket handlers. Here's my detailed feedback:

✅ Strengths

  1. Clear Separation of Concerns: The new RequestContext and WebSocketContext types provide specialized contexts that are more appropriate than the generic ActorContext for these specific use cases.

  2. Better Developer Experience: Exposing the conn property directly on the context makes it more ergonomic for developers to access connection-specific data in raw handlers.

  3. Consistent Pattern: This follows the existing pattern established by ActionContext, creating a consistent API across different handler types.

  4. Good Documentation: The JSDoc comments clearly explain what each context is for and its type parameters.

  5. Backward Compatible: The changes maintain backward compatibility while improving type safety.

🔍 Code Quality Observations

Duplicate Code Pattern

The RequestContext and WebSocketContext classes are nearly identical (183 lines each with exact same implementation). This violates the DRY principle.

Recommendation: Consider extracting a shared base class or using a generic helper to reduce duplication. For example:

// Base context that wraps ActorContext with conn
abstract class ConnContext<TState, TConnParams, TConnState, TVars, TInput, TDatabase> {
  #actorContext: ActorContext<...>;
  
  constructor(actorContext: ActorContext<...>, public readonly conn: Conn<...>) {
    this.#actorContext = actorContext;
  }
  
  // All the delegating getters and methods here
}

export class RequestContext<...> extends ConnContext<...> {}
export class WebSocketContext<...> extends ConnContext<...> {}

This would make the codebase easier to maintain and ensure both contexts remain consistent.

Signature Cleanup

Good job cleaning up the handler signatures:

  • Removing the empty opts: {} parameter from onRequest
  • Moving conn from a method parameter to a context property

This is a more intuitive API design.

🐛 Potential Issues

  1. Missing Error Handling Context: The handlers in instance/mod.ts (lines 661-665 and 693-697) create the contexts but if an error occurs during context creation or handler execution, the error messages might not be as helpful since they won't show which context type failed. This is probably fine, but worth noting.

  2. Type Safety: The use of any in type parameters in the fixture files (raw-http.ts, raw-http-request-properties.ts) is acceptable for test fixtures, but real implementations should use proper types.

🎯 Test Coverage

The existing test suite in tests/raw-http.ts and fixture files have been updated correctly. The tests should verify:

  • ✅ Access to conn property from the new contexts
  • ✅ State management through contexts
  • ✅ Request/WebSocket handling

Since these are structural changes, the existing integration tests should provide adequate coverage.

Optional Enhancement: Consider adding a focused test that explicitly validates the conn property is accessible and contains the expected data in both RequestContext and WebSocketContext.

📋 Minor Suggestions

  1. Export Location (rivetkit-typescript/packages/rivetkit/src/actor/mod.ts:76-77): Good that these are exported from the main module.

  2. Documentation: Consider adding a migration guide or example in the PR description showing the before/after for users who might have existing onRequest/onWebSocket handlers.

🎉 Summary

This is a solid refactor that improves the API design. The main suggestion is to reduce code duplication between the two context classes. The change is well-tested through existing integration tests.

Approval Status: ✅ Approve with minor suggestion to address code duplication in a follow-up PR if desired.


Review generated with assistance from Claude Code

@claude
Copy link

claude bot commented Nov 12, 2025

PR Review: Add WebSocketContext and RequestContext

Summary

This PR introduces two new specialized context classes (RequestContext and WebSocketContext) to provide better type safety and API clarity for raw HTTP and WebSocket handlers in RivetKit. The changes improve the developer experience by making the connection context explicit in these handlers.

Code Quality & Best Practices ✅

Strengths:

  1. Consistent Pattern: The new context classes follow the same delegation pattern as ActionContext, maintaining consistency across the codebase
  2. Type Safety: All generic type parameters are properly threaded through the new context classes
  3. Documentation: Good JSDoc comments on both context classes and updated config signatures
  4. Proper Encapsulation: Uses private fields (#actorContext) to hide implementation details while exposing necessary functionality through public getters
  5. Export Management: Properly exports the new types from the main module entry point (actor/mod.ts)

Code Structure:

  • Both RequestContext and WebSocketContext are nearly identical in implementation, which is appropriate given they serve similar purposes for different connection types
  • The delegation pattern to ActorContext is clean and maintainable
  • Signature changes in config.ts make the API more intuitive by removing the empty opts: {} parameter from onRequest

Potential Issues 🔍

1. Code Duplication (Minor)

The RequestContext and WebSocketContext classes are virtually identical (183 lines each, identical implementation). Consider:

Suggestion: While the duplication is acceptable given the type safety benefits and clear naming, you could explore a shared base class if more context types are added in the future. For now, this is fine.

2. Signature Change - Breaking Change (Important)

The onRequest signature changed from:

onRequest?: (
  c: ActorContext<...>,
  request: Request,
  opts: {},
) => Response | Promise<Response>;

to:

onRequest?: (
  c: RequestContext<...>,
  request: Request,
) => Response | Promise<Response>;

Similarly, onWebSocket changed to accept WebSocketContext instead of ActorContext.

Impact: This is a breaking change for any existing code using these handlers. The fixture files were updated (raw-http.ts, raw-http-request-properties.ts), but this will affect external consumers.

Recommendation:

  • Ensure this is documented in release notes/changelog
  • Consider if a deprecation path would be beneficial (though the clean break seems reasonable given the improved API)

3. Connection Parameter Ordering

In instance/mod.ts, the connection is now passed as the first parameter:

// Before
await this.#config.onRequest(this.actorContext, request, opts);

// After  
const ctx = new RequestContext(this.actorContext, conn);
const response = await this.#config.onRequest(ctx, request);

Observation: This is consistent with how actions work, where the context (with conn as a property) comes first. Good consistency! ✅

4. Connection Lifecycle in router-endpoints.ts

In router-endpoints.ts:handleRawRequest, a connection is created and cleaned up in a try-finally:

try {
    conn = new Conn(actor.id, persist);
    actor.connConnected(conn);
    createdConn = conn;
    return await actor.handleRawRequest(conn, req);
} finally {
    if (createdConn) {
        actor.connDisconnected(createdConn, false);
    }
}

Question: Is it always appropriate to disconnect the connection after a single request? This seems intentional for stateless HTTP requests, but worth confirming this is the desired behavior for all use cases.

Performance Considerations ⚡

  1. Context Object Creation: Each request/WebSocket connection now creates an additional context wrapper object. This is negligible overhead and worth the API improvement.

  2. No Performance Regressions: The changes don't introduce any additional async operations or blocking calls. All delegated methods maintain the same performance characteristics.

Security Concerns 🔒

No security concerns identified. The changes:

  • Don't expose new attack surfaces
  • Maintain the same access control patterns
  • Don't bypass any existing security checks

Test Coverage 📋

Existing Coverage:

  • ✅ Fixture actors updated (raw-http.ts, raw-http-request-properties.ts)
  • ✅ Test files reference these fixtures and should verify the new signature works

Gaps:

  1. Direct Context Testing: While the fixtures use the new contexts, there aren't explicit tests verifying:

    • All context methods are accessible and work correctly
    • The conn property is properly exposed
    • Error cases when context methods are called
  2. Migration Path Testing: No tests verify backward compatibility concerns (though this appears intentional as a breaking change)

Recommendation: Consider adding unit tests specifically for the context classes to verify all delegated methods work correctly.

Additional Observations

Positive Changes:

  1. Clearer Intent: RequestContext vs ActorContext makes it immediately clear what kind of handler you're in
  2. Connection Access: Having ctx.conn readily available is more convenient than searching through ctx.conns map
  3. Future Extensibility: This pattern makes it easy to add request/WebSocket-specific functionality to these contexts in the future without polluting the general ActorContext

Minor Inconsistency:

In config.ts, the JSDoc comment for onWebSocket says:

@param opts Additional options including the original HTTP upgrade request

But the actual signature shows the request is in the opts object, which is correct. The comment could be slightly clearer.

Final Verdict

This is a well-executed refactor that improves type safety and API clarity. The breaking change is justified by the significant improvement in developer experience.

Required Actions:

  • Document breaking changes in changelog/migration guide
  • Verify all internal usages have been updated (fixtures appear complete)

Recommended Actions:

  • Add unit tests for the new context classes
  • Consider adding examples in docs showing the new context usage
  • Clarify JSDoc for onWebSocket opts parameter

Approval Status: ✅ Approve with minor recommendations

The code is production-ready. The breaking change is clean and well-motivated. Test coverage through fixtures appears sufficient, though additional unit tests would be valuable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants