Skip to content

Conversation

@NathanFlurry
Copy link
Member

No description provided.

@vercel
Copy link

vercel bot commented Dec 23, 2025

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

Project Deployment Review Updated (UTC)
rivet-site Ready Ready Preview, Comment Dec 23, 2025 7:56am
rivetkit-serverless Error Error Dec 23, 2025 7:56am
2 Skipped Deployments
Project Deployment Review Updated (UTC)
rivet-cloud Ignored Ignored Dec 23, 2025 7:56am
rivet-inspector Ignored Ignored Preview Dec 23, 2025 7:56am

Copy link
Member Author

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 Dec 23, 2025

Pull Request Review: Specialize Context Types for Better Compiler Errors

Overview

This PR improves the developer experience by creating specialized context types for each lifecycle hook in RivetKit. This provides better type checking and more helpful compiler errors.

✅ Strengths

1. Excellent Developer Experience Improvement

The specialized context types make it much clearer which properties are available in each lifecycle hook:

  • CreateContext and CreateVarsContext correctly use never for unavailable properties
  • Type helpers like ActorContextOf, CreateContextOf, etc. provide excellent type inference from actor definitions
  • Documentation comments added to explain when properties are unavailable

2. Well-Organized Code Structure

The new directory structure is clean and logical:

  • Base contexts in ./base/ directory (actor.ts, conn.ts, conn-init.ts)
  • Lifecycle-specific contexts as separate files
  • Clean barrel export in index.ts

3. Type Safety Improvements

The use of conditional types for optional properties is excellent:

get state(): TState extends never ? never : TState {
  return this.#actor.state as TState extends never ? never : TState;
}

This prevents accessing state in createState where it doesn't exist yet.

4. Consistent Naming Convention

  • Renamed OnBeforeConnectContextBeforeConnectContext
  • Renamed OnConnectContextConnectContext
    This improves consistency (the hooks are onBeforeConnect, onConnect, but the context types drop the "On" prefix).

⚠️ Issues & Concerns

1. Type Safety Issue in ConnInitContext Constructor

Location: rivetkit-typescript/packages/rivetkit/src/actor/contexts/base/conn-init.ts:32

super(actor as any);

Issue: Using as any defeats the purpose of type specialization and could hide type errors.

Root Cause: The ConnInitContext extends ActorContext<TState, never, never, TVars, TInput, TDatabase> but receives an ActorInstance<TState, any, any, TVars, TInput, TDatabase>. The any types for connection parameters don't match never.

Recommendation: Consider one of these approaches:

  1. Update the parent constructor to accept a broader type
  2. Create an intermediate type that allows this conversion safely
  3. Document why as any is necessary here with a comment

2. Inconsistent Whitespace in New Files

Locations: Multiple new context files

Several new files have inconsistent trailing blank lines:

  • create-vars.ts (line 12)
  • create.ts (line 12)
  • destroy.ts (line 23)
  • disconnect.ts (line 24)
  • sleep.ts (line 23)
  • state-change.ts (line 23)
  • wake.ts (line 23)

Recommendation: Remove extra blank lines for consistency with existing code style.

3. Missing JSDoc Comments

Location: New context files

The new context type files lack detailed JSDoc comments. While they have basic descriptions, they could benefit from more comprehensive documentation about:

  • When each context is used
  • What properties are available
  • Examples of usage

Example improvement:

/**
 * Context for the onCreate lifecycle hook.
 * 
 * @remarks
 * This context is available during actor initialization, before state and vars are created.
 * The `state` and `vars` properties are not available in this context.
 * 
 * @example
 * ```typescript
 * onCreate: async (c, input) => {
 *   // c.state is not available here
 *   await c.db.initialize();
 * }
 * ```
 */
export class CreateContext<...> { }

4. Type Complexity

The type helper patterns like CreateContextOf<AD> are excellent, but they add complexity. Consider:

  • Adding examples in comments showing how to use these type helpers
  • Documenting the pattern in a central location for future context additions

5. Potential Breaking Change

Impact: Low to Medium

The renaming of context types (OnBeforeConnectContextBeforeConnectContext, OnConnectContextConnectContext) is a breaking change for any code that explicitly types these contexts.

Recommendation:

  • Verify this is the intended breaking change
  • Consider deprecation warnings if backwards compatibility is needed
  • Ensure this is documented in release notes

📝 Minor Observations

1. Import Path Changes

The change from ./contexts/actor to ./contexts/base/actor is handled well throughout the codebase. Good refactoring work.

2. Documentation Updates

The documentation files in website/src/content/docs/actors/ were updated appropriately to reflect the new context names. Well done!

3. Unused Import in disconnect.ts

Location: rivetkit-typescript/packages/rivetkit/src/actor/contexts/disconnect.ts:2

import type { Conn } from "../conn/mod";

This import appears unused. Consider removing it.

🎯 Recommendations Summary

High Priority

  1. Fix the as any cast in conn-init.ts or document why it's necessary
  2. Verify the breaking change for context renames is intentional

Medium Priority

  1. Remove trailing blank lines for consistency
  2. Remove unused import in disconnect.ts
  3. Add more comprehensive JSDoc comments to new context types

Low Priority

  1. Add usage examples for the *ContextOf type helpers
  2. Consider adding a test that validates the type specialization works as intended

✨ Overall Assessment

This is a high-quality PR that significantly improves the developer experience with RivetKit. The type specialization will provide much better compiler errors and IntelliSense support. The code is well-organized and follows good TypeScript patterns.

The main concerns are minor (code style, documentation) with one moderate issue (the as any cast). Once those are addressed, this will be an excellent addition to the codebase.

Recommendation: Approve with minor changes requested.


Review generated by Claude Code

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