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)
rivet-site Ready Ready Preview Comment Nov 10, 2025 0:22am
rivetkit-serverless Ready Ready Preview Comment Nov 10, 2025 0:22am
2 Skipped Deployments
Project Deployment Preview Comments Updated (UTC)
rivet-cloud Ignored Ignored Nov 10, 2025 0:22am
rivet-inspector Ignored Ignored Preview Nov 10, 2025 0:22am

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.

@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@3428

@rivetkit/cloudflare-workers

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

@rivetkit/core

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

@rivetkit/db

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

@rivetkit/framework-base

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

@rivetkit/next-js

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

@rivetkit/react

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

rivetkit

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

@rivetkit/sql-loader

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

@rivetkit/engine-runner

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

@rivetkit/engine-runner-protocol

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

commit: aedd8f2

@claude
Copy link

claude bot commented Nov 10, 2025

Code Review: Connection Lifecycle Context Restructuring

Summary

This PR refactors the connection lifecycle context handling in RivetKit by:

  1. Introducing a cleaner context hierarchy with dedicated classes for different lifecycle hooks
  2. Moving request access from function parameters to context properties
  3. Simplifying API signatures by reducing parameter counts

Positive Aspects

Architecture & Design

Excellent use of inheritance hierarchy: The new ConnInitContext and ConnContext base classes create a clear and logical separation between connection initialization phases and active connection phases.

Context hierarchy:

  • ConnInitContext (has request, no conn)
    • CreateConnStateContext
    • OnBeforeConnectContext
    • OnConnectContext
  • ConnContext (has conn)
    • ActionContext
    • RequestContext (also has request)
    • WebSocketContext (also has request)

Cleaner API: Moving request from an opts parameter to a context property (c.request) is more ergonomic and consistent with the rest of the API.

Type safety: The request?: Request | undefined typing correctly reflects that requests may not always be present in all connection scenarios.

Implementation Quality

Consistent refactoring: All lifecycle hooks have been updated consistently across the codebase.

Backward compatibility considerations: The migration path is clear, as evidenced by the fixture updates.

Issues & Concerns

🔴 Critical: Incomplete Migration (Blocking)

Location: rivetkit-typescript/packages/rivetkit/fixtures/driver-test-suite/raw-websocket.ts:54

The getRequestInfo handler is commented out with a throw "TODO" statement. This is a runtime bomb that will fail if any test calls this code path.

Recommendation: Either:

  1. Complete the migration by accessing c.request from the context
  2. Remove the getRequestInfo feature entirely if no longer needed
  3. Return a clear error message instead of throwing a bare string

Suggested fix: Access ctx.request to get the request URL info, similar to how it's done in request-access.ts fixture.

⚠️ Minor Issues

1. Empty Type Definition

Location: rivetkit-typescript/packages/rivetkit/src/actor/conn/drivers/websocket.ts:8

export type ConnDriverWebSocketState = Record<never, never>;

While technically correct, this is slightly unusual. Could use Record<string, never> or {} instead.

Impact: Low - it works correctly but is unconventional.

2. Documentation Completeness

The new context classes have basic JSDoc comments, but could benefit from more detailed documentation:

  • Examples of when each context is used
  • Lifecycle ordering diagram
  • Clear explanation of when request will be undefined

3. Type Parameter Verbosity

The generic type parameters are quite verbose across multiple context classes. Consider if some could be simplified, though this may be unavoidable given TypeScript limitations.

Testing Considerations

Good test coverage: The fixture updates show that the migration has been tested across multiple scenarios (conn-params, conn-state, lifecycle, request-access, etc.)

⚠️ Missing: Explicit tests for the request === undefined case to ensure the code handles optional requests gracefully.

Performance Impact

Negligible: The refactoring primarily changes how data is accessed (via context properties vs parameters), with no material performance impact.

Security Review

No concerns: The changes do not introduce new security vulnerabilities. Request access is properly controlled through the context.

Documentation Updates

📚 Action needed: The website/public/llms-full.txt and website/public/llms.txt files were updated, which suggests documentation was addressed. Verify these changes accurately reflect the new API.

Recommendations

Before Merge (Required)

  1. Fix the throw "TODO" in raw-websocket.ts:54 (see detailed suggestion above)
  2. Run the full test suite, especially driver tests, to ensure the TODO does not break tests
  3. Consider adding a test case that explicitly validates request === undefined handling

Future Improvements (Optional)

  1. Add more comprehensive JSDoc examples to the new context classes
  2. Consider creating a migration guide for users updating their actors
  3. Document the lifecycle ordering and when each context type is used

Verdict

Status: ⚠️ Needs Changes

The refactoring is well-designed and improves the API significantly, but the incomplete migration in raw-websocket.ts must be addressed before merging. Once the TODO is resolved and tests pass, this should be good to merge.

Estimated effort to fix: ~5-10 minutes

@claude
Copy link

claude bot commented Nov 10, 2025

Code Review: Connection Lifecycle Context Restructuring

This PR refactors the RivetKit connection lifecycle contexts to improve type safety and reduce code duplication. Overall, the changes are well-structured and follow good architectural patterns.

Strengths

  1. Excellent Use of Inheritance Hierarchy: The new context class hierarchy is well-designed with ConnInitContext as the base for initialization hooks and ConnContext as the base for established connections.

  2. Reduced Code Duplication: The refactoring eliminates approximately 300+ lines of duplicated code across ActionContext, RequestContext, and WebSocketContext by using inheritance from ActorContext.

  3. Improved API Consistency: Removing the opts parameter and moving request to the context object creates a cleaner, more intuitive API.

  4. Better Type Safety: The inheritance structure ensures that request is only available where it makes sense.

  5. Good Documentation: New context classes have clear JSDoc comments explaining their purpose.

Issues and Concerns

1. Breaking Change with TODO Comment (HIGH PRIORITY)

In rivetkit-typescript/packages/rivetkit/fixtures/driver-test-suite/raw-websocket.ts:54, the code now has throw TODO where request info should be returned. This test fixture now throws an error instead of providing working functionality.

Recommendation: Either complete the implementation using c.request, remove this test case if no longer relevant, or add a proper error message.

2. Request Availability Not Clear (MEDIUM PRIORITY)

The request field is marked as Request | undefined in multiple contexts, but the documentation does not clearly explain when it will be undefined and what users should check before using it.

Recommendation: Add clearer documentation explaining that request will be defined for connections initiated via HTTP/WebSocket upgrade requests, but undefined for connections restored from hibernation.

3. Empty Type Definition (LOW PRIORITY)

In websocket.ts:8, changed from empty object to Record never never. While more explicit, the old empty object was clearer for representing an empty object type.

4. Type Safety in ConnectionManager (LOW PRIORITY)

In connection-manager.ts, there are several as any casts at lines 153, 275, and 286. Consider adding proper type definitions to avoid type assertions.

Questions

  1. Are there breaking changes for existing users? Should there be migration notes?
  2. Are there tests that verify request is properly passed through all contexts?

Performance and Security

  • Performance: No concerns. The inheritance-based approach should have minimal overhead.
  • Security: No issues identified. The request object handling appears safe.

Recommendations Summary

Before Merge:

  1. Fix or remove the throw TODO in raw-websocket.ts
  2. Improve documentation for when request is undefined
  3. Verify all tests pass

Nice to Have:
4. Add type safety to avoid as any casts in ConnectionManager
5. Consider keeping empty object instead of Record never never for clarity

Overall Assessment

This is a solid refactoring that improves code quality and API design. The inheritance hierarchy is well-thought-out and the reduction in code duplication is significant. The main blocker is the incomplete TODO implementation.

Recommendation: Request changes for the TODO issue, then approve.

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