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 Jan 2, 2026 11:37pm
rivetkit-serverless Error Error Jan 2, 2026 11:37pm
2 Skipped Deployments
Project Deployment Review Updated (UTC)
rivet-cloud Ignored Ignored Jan 2, 2026 11:37pm
rivet-inspector Ignored Ignored Preview Jan 2, 2026 11:37pm

Copy link
Member Author

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

PR Review: Specialize Context Types for Better Compiler Errors

Summary

This PR refactors RivetKit's context type system to provide more specialized context types for different lifecycle hooks. This is a significant improvement that will provide better TypeScript developer experience through stronger type safety and clearer APIs.

✅ Strengths

1. Well-Structured Type Hierarchy

The new context hierarchy is clean and logical:

  • Base contexts (ActorContext, ConnContext, ConnInitContext) in /base/ directory
  • Specialized lifecycle contexts (CreateContext, DestroyContext, WakeContext, etc.)
  • Type extractors for each context (CreateContextOf, ActionContextOf, etc.)

2. Type Safety Improvements

The use of never types for unavailable properties ensures developers get proper TypeScript errors when trying to access properties that aren't available in a given lifecycle hook.

3. Clear Documentation

Added JSDoc comments clarifying when properties are/aren't available in different contexts.

4. Proper Context Specialization

Each lifecycle hook now has a context that reflects what's actually available:

  • CreateContext<TState, TInput, TDatabase> - No conn params/state, no vars
  • CreateVarsContext<TState, TInput, TDatabase> - Before vars exist
  • BeforeConnectContext<TState, TVars, TInput, TDatabase> - State and vars available, but no conn
  • ConnectContext - Full context with connections

5. Good File Organization

  • Base contexts in contexts/base/
  • Each lifecycle hook gets its own file
  • Central index.ts for clean imports
  • Consistent naming (*Context and *ContextOf)

🔍 Issues & Recommendations

1. Type Casting Could Hide Runtime Errors ⚠️

In ConnInitContext line 32: super(actor as any);

Recommendation: Add a comment explaining why this cast is safe, or refactor to avoid it.

2. Inconsistent Blank Lines (Minor)

Several files have double blank lines before the *ContextOf type export.

Files: create.ts:14, create-vars.ts:13, destroy.ts:23, disconnect.ts:24, sleep.ts:23, state-change.ts:23, wake.ts:23, request.ts:50, websocket.ts:50

3. Documentation Could Be More Specific

The JSDoc comments are generic. Consider adding details about what's available/unavailable in each context.

4. Missing Type Tests 🧪

Add type tests for the new *ContextOf utilities (similar to existing ActorContextOf test).

5. Consider Runtime Validation 🛡️

Consider adding dev-mode runtime guards to catch access to unavailable properties when TypeScript is bypassed.

6. Potential Breaking Change ⚠️

Removal of InitContext and renaming OnBeforeConnectContextBeforeConnectContext and OnConnectContextConnectContext could break external consumers. Consider adding deprecated aliases if needed.

🎯 Performance & Security

No runtime performance impact - purely type-level changes
No security issues identified

📊 Test Coverage

Current: One type test for ActorContextOf
Recommendation: Add comprehensive type tests for all new *ContextOf extractors

Manual Testing Suggested:

  1. Access c.vars in createVars - should error
  2. Access c.state in createState - should error
  3. Access c.db without db config - should error
  4. Verify autocomplete in each lifecycle hook

📚 Documentation Updates

✅ Documentation is consistent with code changes

Summary & Recommendation

Overall Assessment: ✅ APPROVE with minor suggestions

This is a high-quality refactor that significantly improves developer experience. The type system changes are well thought out and will prevent many runtime errors at compile time.

Priority Issues:

  1. Medium: Add comment explaining as any cast (Issue [SVC-2555] Set up issue templates #1)
  2. Low: Fix double blank lines (Issue [SVC-2479] Send cluster events to PostHog #2)
  3. Low: Add type tests (Issue [SVC-2483] Remove hardcoded uses of rivet.gg #4)

Optional:

The core implementation is solid and ready to merge. Great work! 🎉

@NathanFlurry NathanFlurry force-pushed the 12-23-chore_rivetkit_specialize_context_types_for_better_compiler_errors branch from f96b9d8 to e4a1e83 Compare January 2, 2026 23:59
@jog1t jog1t force-pushed the 12-23-chore_rivetkit_specialize_context_types_for_better_compiler_errors branch from e4a1e83 to f96b9d8 Compare January 3, 2026 00:05
@jog1t jog1t force-pushed the 12-22-fix_rivetkit_fix_c.client_not_respecting_correct_origin branch from 81d8c74 to 49e0689 Compare January 3, 2026 00:05
@NathanFlurry NathanFlurry force-pushed the 12-22-fix_rivetkit_fix_c.client_not_respecting_correct_origin branch from 49e0689 to 81d8c74 Compare January 5, 2026 02:45
@NathanFlurry NathanFlurry force-pushed the 12-23-chore_rivetkit_specialize_context_types_for_better_compiler_errors branch from f96b9d8 to e4a1e83 Compare January 5, 2026 02:45
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