Skip to content

Conversation

@NathanFlurry
Copy link
Member

No description provided.

@vercel
Copy link

vercel bot commented Nov 13, 2025

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

Project Deployment Preview Comments Updated (UTC)
rivetkit-serverless Error Error Nov 13, 2025 10:39pm
3 Skipped Deployments
Project Deployment Preview Comments Updated (UTC)
rivet-cloud Ignored Ignored Nov 13, 2025 10:39pm
rivet-inspector Ignored Ignored Preview Nov 13, 2025 10:39pm
rivet-site Ignored Ignored Preview Nov 13, 2025 10:39pm

Copy link
Member Author

NathanFlurry commented Nov 13, 2025


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 13, 2025

PR Review: Type Check Fixes

Summary

This PR addresses TypeScript type checking issues in the RivetKit TypeScript package by updating lifecycle hook names and improving type safety around the request context. The changes primarily involve:

  • Renaming lifecycle hooks from onStart/onStop to onWake/onSleep
  • Renaming onFetch to onRequest
  • Adding null checks for request context
  • Removing deprecated transport configuration
  • Reformatting AsyncAPI spec

Code Quality & Best Practices

Positive:

  • Consistent API naming: The lifecycle hook renames (onWake/onSleep) better convey the actor lifecycle semantics
  • Type safety improvements: Adding null checks for c.request (e.g., in cursors-raw-websocket/registry.ts:60) improves runtime safety
  • Reduced API surface: Removing the transport parameter simplifies the client configuration
  • Proper error handling: The new null check throws a clear error message: throw "Missing request"

Concerns:

  • ⚠️ Error handling pattern: In cursors-raw-websocket/registry.ts:60, throwing a string (throw "Missing request") instead of an Error object is non-standard JavaScript practice. Consider:

    if (!c.request) throw new Error("Missing request");

    This provides proper stack traces and is more idiomatic.

  • ⚠️ Breaking API changes: The lifecycle hook renames (onStartonWake, onStoponSleep, onFetchonRequest) are breaking changes. Ensure:

    1. Version bump reflects this (major version if following semver)
    2. Migration guide exists for users
    3. Deprecation warnings were issued in previous versions if possible

Potential Bugs

  • ⚠️ Incomplete signature migration: In kitchen-sink/websocket.ts, the function signature changed from:

    // Before
    export function handleWebSocket(c: any, websocket: UniversalWebSocket, opts: any)
    // After  
    export function handleWebSocket(c: any, websocket: UniversalWebSocket)

    The opts.request.url was replaced with c.request.url. However, there's no null check here like in the cursors example. This could cause a runtime error if c.request is undefined. Add:

    if (!c.request) throw new Error("Missing request");
    const url = c.request.url;
  • ⚠️ Cloudflare Workers driver: In cloudflare-workers/actor-driver.ts:290-300, the startDestroy method calls actor.onStop("destroy") but the lifecycle hook was renamed to onSleep. Verify this is intentional or if it needs to be updated to match the new naming convention.


Performance Considerations

  • AsyncAPI formatting: The large diff in asyncapi.json appears to be purely formatting (spaces → tabs), which has no performance impact
  • No observable performance regressions: The changes are primarily type-related and don't introduce new computational overhead

Security Concerns

  • ⚠️ Request validation: The added null checks for c.request improve security by preventing potential null pointer exceptions. However, consider:
    1. When/why would c.request be null? Document this behavior
    2. If it's an invariant that should always exist, consider asserting it earlier in the request pipeline
    3. The error message "Missing request" could be more descriptive for debugging

Test Coverage

Observations:

  • ✅ Tests updated in engine/tests/smoke/src/server/registry.ts to use new hook names
  • ✅ Examples updated across multiple files showing the API changes work

Recommendations:

  • Test coverage: It's unclear if there are tests covering:
    1. The c.request null case - does this scenario occur in practice?
    2. Migration path for the breaking changes
    3. Backward compatibility (if any is intended)

Consider adding tests for edge cases around the request context.


Additional Notes

AsyncAPI Spec Changes:
The asyncapi.json diff is primarily formatting (switching from 2-space to tab indentation). While this is fine, consider:

  • Using a consistent formatting tool (e.g., Prettier with project config) to prevent future formatting churn
  • Committing formatting changes separately from functional changes for easier review

Frontend Cleanup:
Good removal of unused transport configuration from the frontend components, reducing complexity for users.

Type Safety in node.ts:
The changes to node.ts appear to improve the dynamic import handling with better error messages. Good addition of JSDoc comments explaining the design decisions around using require() vs import().


Recommendations Summary

  1. High Priority:

    • Fix error throwing to use Error objects: throw new Error(...) instead of throw "..."
    • Add null check for c.request in kitchen-sink/websocket.ts:14
    • Verify the onStoponSleep migration in Cloudflare driver
  2. Medium Priority:

    • Document when c.request can be null/undefined
    • Ensure version bump and migration docs exist
    • Add test coverage for request null cases
  3. Low Priority:

    • Use consistent formatting tools to avoid future formatting-only PRs
    • Consider more descriptive error messages

Verdict

This PR makes valuable improvements to type safety and API consistency. The main concerns are around ensuring proper error handling patterns and verifying all lifecycle hook renames are complete. Once the error throwing pattern is fixed and the potential null reference in websocket.ts is addressed, this should be good to merge.

Overall: Approve with suggested changes


Review generated by Claude Code

@pkg-pr-new
Copy link

pkg-pr-new bot commented Nov 13, 2025

More templates

@rivetkit/cloudflare-workers

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

@rivetkit/db

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

@rivetkit/framework-base

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

@rivetkit/next-js

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

@rivetkit/react

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

rivetkit

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

@rivetkit/sql-loader

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

@rivetkit/engine-runner

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

@rivetkit/engine-runner-protocol

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

commit: 2e4753e

@NathanFlurry NathanFlurry force-pushed the 11-12-chore_rivetkit_fix_type_checks branch from 2e4753e to 9031170 Compare November 13, 2025 17:13
@NathanFlurry NathanFlurry force-pushed the 11-12-feat_rivetkit_add_ability_to_destroy_actors branch from 3985a9e to 8ea24ef Compare November 13, 2025 17:13
@claude
Copy link

claude bot commented Nov 13, 2025

PR Review: chore(rivetkit): fix type checks

Summary

This PR fixes TypeScript type errors across RivetKit by updating lifecycle hooks and API method names to match the current API. The changes reflect a migration from onStart/onStop to onWake/onSleep, and from onFetch to onRequest.


✅ Positive Observations

  1. Consistent API Updates: The changes correctly align with the lifecycle hook naming convention update (onWake/onSleep instead of onStart/onStop).

  2. Proper Context Access: The fix in cursors-raw-websocket/src/backend/registry.ts correctly accesses the request from the context object (c.request) instead of a nonexistent standalone request variable.

  3. Code Cleanup: Removal of transport-related code from the frontend examples simplifies the API surface and removes unnecessary configuration options.

  4. Type Safety: The changes fix TypeScript compilation errors, improving type safety across the codebase.

  5. Formatting Consistency: The AsyncAPI JSON file was reformatted with tabs, which is consistent with the project's formatting standards.


🔍 Issues and Concerns

1. Error Handling - Critical Issue ⚠️

File: examples/cursors-raw-websocket/src/backend/registry.ts:60

if (!c.request) throw "Missing request";

Issue: Throwing a string instead of an Error object is bad practice and makes error handling more difficult.

Recommendation:

if (!c.request) throw new Error("Missing request");

Severity: Medium - This should be fixed before merging.


2. Unsafe Function Signature ⚠️

File: examples/kitchen-sink/src/backend/actors/websocket.ts:2

export function handleWebSocket(c: any, websocket: UniversalWebSocket) {

Issue: Using any type defeats the purpose of TypeScript's type system and could hide bugs.

Recommendation: Define a proper type for the context parameter:

import type { ActorContext } from 'rivetkit';

export function handleWebSocket(
  c: ActorContext<typeof demo>,
  websocket: UniversalWebSocket
) {

Severity: Medium - Type safety issue that should be addressed.


3. Missing Null Check in utils/node.ts

File: rivetkit-typescript/packages/rivetkit/src/utils/node.ts:73-78

Observation: The importNodeDependencies() function catches errors and logs a warning, but then re-throws. This is good, but the error is caught generically.

Suggestion: Consider adding more specific error handling for different failure modes (e.g., module not found vs. other errors).


4. Test Coverage

Concern: While the PR fixes type errors, there's no indication that tests were run or updated to verify the behavior changes (especially the lifecycle hook renames).

Recommendation:

  • Run the existing test suite to ensure nothing broke
  • Consider adding tests that specifically verify the onWake/onSleep lifecycle
  • Add tests for the request context access pattern

5. Breaking Changes Documentation

Issue: The changes from onFetch to onRequest and onStart/onStop to onWake/onSleep are breaking API changes for users.

Recommendation: Ensure these changes are:

  • Documented in a migration guide
  • Called out in the changelog
  • Potentially include deprecation warnings if backward compatibility is needed

📋 Minor Observations

  1. AsyncAPI File Changes: The large diff in rivetkit-asyncapi/asyncapi.json is purely formatting (spaces to tabs). While this is fine, it makes the PR harder to review. Consider doing formatting changes in separate PRs.

  2. Console.log Usage: In cursors-raw-websocket/src/backend/registry.ts:69, there's a console.log statement. Consider using a proper logging framework for consistency.

  3. Function Parameter Removal: The opts parameter was removed from handleWebSocket. Verify this doesn't break any other code that might be calling this function.


🔒 Security Considerations

No significant security issues identified. The changes are primarily type-related refactoring.


⚡ Performance Considerations

No performance concerns. The changes are compile-time only and don't affect runtime performance.


📝 Recommendations Summary

Must Fix Before Merge:

  1. Replace throw "Missing request" with throw new Error("Missing request")
  2. Run the test suite to verify no regressions
  3. Replace any type in handleWebSocket with proper typing

Should Consider:

  1. Add/update tests for lifecycle hooks
  2. Document breaking API changes
  3. Consider separating formatting changes from functional changes in future PRs

Overall Assessment

This PR makes necessary type safety improvements and API consistency updates. However, there are a few issues that should be addressed before merging, particularly around error handling and type safety. Once those are fixed, this should be good to merge.

Status: ⚠️ Needs Changes - Address the error handling and type safety issues mentioned above.

@claude
Copy link

claude bot commented Nov 13, 2025

PR Review: Type Check Fixes

This PR addresses TypeScript type checking issues across the RivetKit codebase. Overall, the changes are clean and consistent, successfully improving type safety. Here's my detailed review:

✅ Strengths

  1. Consistent API Renaming: The lifecycle hook renames (onStartonWake, onStoponSleep) and request handler rename (onFetchonRequest) are applied consistently across all examples and implementations.

  2. Improved Type Safety: The changes in examples/cursors-raw-websocket/src/backend/registry.ts properly handle the optional c.request field with a null check before use, preventing potential runtime errors.

  3. Simplified WebSocket Handler: Removing the unused opts parameter in handleWebSocket (examples/kitchen-sink/src/backend/actors/websocket.ts) and accessing c.request directly is cleaner and more consistent with the framework's API.

  4. Code Cleanup: Removing the transport selection UI in the kitchen-sink example simplifies the codebase, which likely aligns with framework changes.

  5. AsyncAPI Formatting: The AsyncAPI JSON file has been reformatted with tabs for consistency.

🔍 Observations & Recommendations

1. Error Handling in cursors-raw-websocket (Line 60)

if (!c.request) throw "Missing request";

Observation: Using string literal throws is not a best practice.

Recommendation: Consider using a proper Error object for better stack traces and debugging:

if (!c.request) throw new Error("Missing request");

2. Node.js Module Loading Strategy

In rivetkit-typescript/packages/rivetkit/src/utils/node.ts, the use of synchronous require() is well-documented and justified in the comments. However, there are a few points to consider:

Good:

  • Clear documentation explaining why synchronous loading is necessary
  • Proper error handling with warnings
  • Lazy loading pattern to avoid loading in non-Node environments

Potential Concern:

  • Lines 54-70 use // @ts-ignore extensively. While necessary for dynamic requires, consider if there's a more type-safe approach using conditional types or module augmentation.

Minor: The webpack ignore comments (/* webpackIgnore: true */) are helpful for bundlers.

3. Cloudflare Workers onStop Reference

In rivetkit-typescript/packages/cloudflare-workers/src/actor-driver.ts:300, there's a call to actor.onStop("destroy") which uses the old naming convention. This should likely be:

// Line 300 - Should this be onSleep instead of onStop?
await actor.onStop("destroy");

Action Required: Verify if this internal method name was intentionally kept as onStop or if it should also be renamed to align with the new lifecycle naming.

4. AsyncAPI Changes

The AsyncAPI file changes appear to be purely cosmetic (spacing/formatting). Verify that:

  • The AsyncAPI spec is still valid
  • Any tooling consuming this file works correctly with tab formatting
  • The version number (2.0.24-rc.1) is correct

5. Missing Test Coverage

Observation: The PR doesn't include any test file updates.

Recommendation: Consider verifying that:

  • Existing tests still pass with the renamed lifecycle hooks
  • The WebSocket request access pattern is covered by tests
  • The error handling for missing c.request is tested

🔒 Security

No security concerns identified. The changes improve type safety which indirectly enhances security.

⚡ Performance

No performance impact expected. The changes are primarily type-related and API renames.

🎯 Summary

Verdict: This is a solid cleanup PR that improves type safety across the codebase.

Before Merge:

  1. ✅ Verify the actor.onStop() call in cloudflare-workers/src/actor-driver.ts:300 is correct
  2. ⚠️ Consider using new Error() instead of string throws
  3. ⚠️ Verify AsyncAPI file is still valid and consumed correctly by tooling
  4. 📝 Confirm test suite passes with the new API names

Nice to have:

  • Add JSDoc comments explaining the lifecycle hook name changes for future maintainers
  • Consider adding deprecation notices if the old names might still be referenced elsewhere

Great work on maintaining consistency across the codebase! 🎉

@NathanFlurry NathanFlurry force-pushed the 11-12-feat_rivetkit_add_ability_to_destroy_actors branch from 8ea24ef to a3beed3 Compare November 13, 2025 20:53
@NathanFlurry NathanFlurry force-pushed the 11-12-chore_rivetkit_fix_type_checks branch from 9031170 to 007bec8 Compare November 13, 2025 20:54
@vercel
Copy link

vercel bot commented Nov 13, 2025

Deployment failed with the following error:

Too many requests - try again in 1 minute (more than 120, code: "api-deployments-flood-pro").

Learn More: https://vercel.com/docs/limits#rate-limits

@claude
Copy link

claude bot commented Nov 13, 2025

PR Review: chore(rivetkit): fix type checks

Summary

This PR addresses TypeScript type checking issues across the RivetKit codebase. The changes primarily involve:

  1. Lifecycle hook renames (onStart/onStoponWake/onSleep)
  2. API naming consistency (onFetchonRequest)
  3. Request object access patterns
  4. Removal of deprecated transport options
  5. AsyncAPI schema formatting improvements

Code Quality & Best Practices

✅ Positive Observations

  1. Consistent API Naming: The rename from onFetch to onRequest is more semantically accurate and consistent with modern web standards.

  2. Safer Request Access: The change from opts.request to c.request with null checking (if (!c.request) throw "Missing request") is good defensive programming.

  3. Cleaner Function Signatures: Removing the unnecessary opts parameter from handleWebSocket simplifies the API surface.

  4. AsyncAPI Schema Formatting: The reformatting of asyncapi.json with tabs improves consistency with the project's formatting standards.

⚠️ Concerns & Recommendations

1. Error Handling - Critical Issue

Location: examples/cursors-raw-websocket/src/backend/registry.ts:60

if (!c.request) throw "Missing request";

Issue: Throwing a string instead of an Error object is an anti-pattern.

Recommendation:

if (!c.request) throw new Error("Missing request");

Throwing strings:

  • Prevents proper stack traces
  • Breaks error handling patterns that check instanceof Error
  • Makes debugging more difficult

2. Type Safety - Missing Types

Location: examples/kitchen-sink/src/backend/actors/websocket.ts:3

export function handleWebSocket(c: any, websocket: UniversalWebSocket) {

Issue: Using any type defeats the purpose of TypeScript and this type-checking PR.

Recommendation: Define proper types for the context parameter. Based on the usage, it should have at least:

interface WebSocketContext {
  state: {
    connectionCount?: number;
    messageCount?: number;
    messageHistory?: Array<any>;
  };
  log: {
    info: (msg: string, data?: any) => void;
    error: (msg: string, data?: any) => void;
  };
  request: {
    url: string;
  };
}

export function handleWebSocket(c: WebSocketContext, websocket: UniversalWebSocket) {

3. AsyncAPI Schema Changes

Location: rivetkit-asyncapi/asyncapi.json

Observation: The diff shows 435 additions and 488 deletions, but most appear to be formatting changes (switching from 2 spaces to tabs).

Recommendation:

  • Verify that the functional content of the schema hasn't changed unintentionally
  • Consider using a JSON diff tool to ensure only formatting changed
  • If this is a large formatting-only change, consider mentioning it explicitly in the PR description

4. Lifecycle Hook Renames

Location: engine/tests/smoke/src/server/registry.ts:10-12

onWake: async () => { /* ... */ },
onSleep: async () => { /* ... */ },

Question: This is a breaking API change. Should this be:

  • Documented in a migration guide?
  • Part of a larger version bump?
  • Communicated to users of the SDK?

The onWake/onSleep naming is more semantically appropriate for actor lifecycle management than onStart/onStop, but existing code will break.

Performance Considerations

No significant performance concerns identified. The changes are primarily type-safety and API consistency improvements.

Security Concerns

Low Risk Issue: Error Message Information Disclosure

The error throwing pattern throw "Missing request" doesn't expose sensitive information, but establishing a pattern of proper Error objects is important for future security-sensitive error handling.

Test Coverage

Concern: The PR modifies test files and examples but doesn't show:

  1. New tests for the renamed hooks
  2. Tests verifying the c.request null check behavior
  3. Migration tests ensuring backward compatibility or clear breaking change documentation

Recommendation: Add tests covering:

  • The new lifecycle hooks (onWake, onSleep)
  • Request object null safety
  • Error cases when c.request is undefined

Additional Recommendations

  1. Breaking Changes: If onFetchonRequest and onStart/onStoponWake/onSleep are breaking changes, consider:

    • Updating CHANGELOG
    • Version bump (following semver)
    • Migration guide for users
  2. Consistency Check: Verify all examples and documentation are updated to reflect these API changes.

  3. TypeScript Strict Mode: Consider enabling stricter TypeScript checks to catch any types and missing null checks earlier.

Verdict

The PR successfully addresses type checking issues and improves API consistency. However, before merging:

  1. MUST FIX: Replace string throwing with proper Error objects
  2. SHOULD FIX: Replace any types with proper type definitions
  3. SHOULD VERIFY: Confirm AsyncAPI schema functional equivalence
  4. SHOULD DOCUMENT: Breaking changes and migration path

Overall, this is solid cleanup work that improves the codebase's type safety, but needs a few fixes before merging.

@graphite-app
Copy link
Contributor

graphite-app bot commented Nov 13, 2025

Merge activity

@NathanFlurry NathanFlurry force-pushed the 11-12-feat_rivetkit_add_ability_to_destroy_actors branch from a3beed3 to 7a3a586 Compare November 13, 2025 21:39
@NathanFlurry NathanFlurry force-pushed the 11-12-chore_rivetkit_fix_type_checks branch from 007bec8 to 1da30ef Compare November 13, 2025 21:39
@claude
Copy link

claude bot commented Nov 13, 2025

PR Review: chore(rivetkit): fix type checks

Summary

This PR fixes TypeScript type checking issues by:

  1. Updating lifecycle hook names from onStart/onStop to onWake/onSleep
  2. Renaming onFetch to onRequest
  3. Simplifying WebSocket handler signatures
  4. Removing deprecated transport option from frontend
  5. Reformatting AsyncAPI schema (tabs to spaces)
  6. Adding proper Node.js module imports

Code Quality ✅

Positive aspects:

  • Consistent API naming improvements (onWake/onSleep better reflect actor lifecycle semantics)
  • Good simplification of WebSocket handler signatures (removing unnecessary opts parameter)
  • Proper null checking added in cursors example (c.request validation)
  • Clean removal of deprecated transport configuration from UI

Potential Issues ⚠️

1. Error Handling - String Throw Anti-pattern

Location: examples/cursors-raw-websocket/src/backend/registry.ts:60

if (!c.request) throw "Missing request";

Issue: Throwing a string instead of an Error object is an anti-pattern. It loses stack traces and type information.

Recommendation: Use proper Error objects:

if (!c.request) throw new Error("Missing request");

2. Type Safety - any Types

Location: examples/kitchen-sink/src/backend/actors/websocket.ts:3,15

export function handleWebSocket(c: any, websocket: UniversalWebSocket) {
  // ...
  url: c.request.url,  // c.request is accessed without type checking
}

Issue: Using any type bypasses TypeScript's type checking. The code accesses c.request without validation that it exists.

Recommendation:

  • Add proper type definitions or at least validate c.request exists before accessing it
  • Consider using the same null check pattern as in the cursors example

3. AsyncAPI Schema Changes

Location: rivetkit-asyncapi/asyncapi.json

Observation: The diff shows significant formatting changes (451 additions, 535 deletions) but appears to be purely whitespace/formatting (spaces vs tabs). The actual schema structure seems unchanged.

Recommendation: Consider using a JSON formatter/linter in your CI to prevent these formatting-only changes in the future.

Performance Considerations 👍

No performance issues identified. The changes are primarily API naming updates and type fixes.

Security Concerns 🔒

No security issues identified. The changes are internal API improvements.

Test Coverage 📋

Observations:

  • Changes update smoke test to use new lifecycle hooks
  • No new tests added, which is appropriate for a refactoring PR
  • Example apps updated to match new API

Recommendation: Ensure existing test suite passes with these API changes.

Breaking Changes 🔴

Important: This PR introduces breaking API changes:

  • onStartonWake
  • onStoponSleep
  • onFetchonRequest
  • WebSocket handler signature change (removed opts parameter)
  • Removed transport option from client config

Recommendation: If this is a public API:

  1. Consider if this should be a major version bump
  2. Add migration notes to CHANGELOG
  3. Consider deprecation warnings before removal if possible

Node.js Module Import Improvement 👍

Location: rivetkit-typescript/packages/rivetkit/src/utils/node.ts:64-70

Good addition of nodeStream import with proper webpack ignore comments. The comments explaining why synchronous require() is used are excellent.

Overall Assessment ✅

This is a clean refactoring PR that improves API naming consistency and fixes type checking issues. The main concerns are:

  1. Error handling pattern (throw string)
  2. any types in websocket handler
  3. Large AsyncAPI formatting change

Recommendation: Approved with minor suggested improvements above.

@claude
Copy link

claude bot commented Nov 13, 2025

PR Review: Type Check Fixes

I've reviewed this PR which focuses on fixing type checks in the RivetKit TypeScript codebase. Overall, the changes are solid and improve type safety. Here's my detailed analysis:

✅ Positive Changes

1. Lifecycle Hook Renaming (onStart/onStop → onWake/onSleep)

  • File: engine/tests/smoke/src/server/registry.ts
  • Better naming that aligns with actor lifecycle semantics
  • onWake and onSleep more accurately describe the actor state transitions
  • Consistent with the rest of the codebase

2. HTTP Handler Renaming (onFetch → onRequest)

  • Files: examples/kitchen-sink/src/backend/actors/demo.ts, examples/raw-fetch-handler/src/backend/registry.ts
  • More generic and clearer naming
  • Better represents that it handles any HTTP request, not just fetch operations

3. WebSocket Handler Signature Improvements

  • File: examples/kitchen-sink/src/backend/actors/websocket.ts
  • Removed unnecessary opts parameter from function signature (line 3)
  • Changed opts.request.url to c.request.url (line 15) - cleaner API surface
  • Type safety improvement by using the context object directly

4. Request Validation

  • File: examples/cursors-raw-websocket/src/backend/registry.ts:60-61
  • Added proper null check: if (!c.request) throw "Missing request";
  • ⚠️ Issue: While this check is good, throwing a string is not ideal (see concerns below)

5. Frontend Cleanup

  • Files: Multiple frontend files
  • Removed deprecated transport parameter from client configuration
  • Simplified the API by removing unused UI controls
  • Net reduction of 26 lines in ConnectionScreen.tsx

6. AsyncAPI JSON Formatting

  • File: rivetkit-asyncapi/asyncapi.json
  • Changed from 2-space to tab indentation
  • More consistent formatting (reduced line count from formatting changes)
  • No functional changes to the schema

7. Node.js Utils Enhancement

  • File: rivetkit-typescript/packages/rivetkit/src/utils/node.ts:22-34
  • Added better comments explaining the synchronous require pattern
  • Improved getRequireFn() helper documentation

8. Cloudflare Workers Driver

  • File: rivetkit-typescript/packages/cloudflare-workers/src/actor-driver.ts:290-291
  • Comment updated to reference onStop (consistent naming)
  • Code correctly calls actor.onStop("destroy") at line 300

⚠️ Concerns & Suggestions

1. Error Handling Anti-Pattern

  • Location: examples/cursors-raw-websocket/src/backend/registry.ts:60
  • Issue: throw "Missing request" throws a string instead of an Error object
  • Recommendation: Change to throw new Error("Missing request") for proper error handling with stack traces

2. Missing Type Safety

  • Location: examples/kitchen-sink/src/backend/actors/websocket.ts:3
  • Issue: Function uses c: any type
  • Recommendation: Define a proper type for the context parameter to improve type safety

3. Test Coverage

  • No test files were modified in this PR
  • Recommendation: Verify that existing tests pass with these API changes, especially the lifecycle hook renamings

4. Breaking Changes

  • The lifecycle hook renaming (onStart/onStoponWake/onSleep) is a breaking change
  • The onFetchonRequest rename is also breaking
  • Recommendation: Ensure this is documented in release notes and migration guides

5. Cloudflare Driver Discrepancy

  • Location: rivetkit-typescript/packages/cloudflare-workers/src/actor-driver.ts
  • Issue: Comment at line 290 says "onStop" but the lifecycle was renamed to "onSleep"
  • The actual call at line 300 uses actor.onStop("destroy") - this suggests the internal method might still be onStop while the user-facing hook is onSleep
  • Recommendation: Clarify this naming in comments or ensure consistency

📊 Code Quality Metrics

  • Lines Added: 451
  • Lines Deleted: 535
  • Net Change: -84 lines (good cleanup!)
  • Files Changed: 12
  • Type Safety: Improved with null checks and cleaner API signatures
  • Breaking Changes: Yes (lifecycle hooks and fetch handler)

🔒 Security

No security concerns identified in this PR. The changes are primarily refactoring and type safety improvements.

🎯 Performance

No performance concerns. The changes are API-level refactoring without algorithmic modifications.

📝 Recommendations

  1. Fix the string throw: Change throw "Missing request" to throw new Error("Missing request")
  2. Add types: Replace any types with proper interfaces where possible
  3. Run tests: Ensure all tests pass, especially integration tests for lifecycle hooks
  4. Update docs: Document the breaking changes in CHANGELOG and migration guide
  5. Verify consistency: Check that the onStop vs onSleep naming is intentional in the Cloudflare driver

✅ Conclusion

This is a solid refactoring PR that improves type safety and API clarity. The main concern is the error handling anti-pattern and ensuring breaking changes are properly documented. Once the string throw is fixed, this should be good to merge.

Overall Assessment: ✅ Approve with minor fixes recommended

@claude
Copy link

claude bot commented Nov 13, 2025

Pull Request Review: chore(rivetkit): fix type checks

Summary

This PR addresses TypeScript type checking issues in RivetKit by:

  1. Renaming lifecycle hooks to match updated API conventions (onStart/onStoponWake/onSleep, onFetchonRequest)
  2. Fixing WebSocket handler signatures to remove unused parameters
  3. Adding proper null checks for c.request in WebSocket handlers
  4. Removing deprecated transport options from the frontend
  5. Updating AsyncAPI specification formatting (tabs → spaces)

Code Quality ✅

Positive aspects:

  • Clear, focused changes that address type errors without adding new functionality
  • Consistent API naming conventions across examples and tests
  • Proper null safety with if (!c.request) throw "Missing request" check

Areas for consideration:

  1. Error handling in cursors-raw-websocket (examples/cursors-raw-websocket/src/backend/registry.ts:60):

    if (!c.request) throw "Missing request";
    • Throwing a string instead of an Error object is not ideal
    • Consider: throw new Error("Missing request") for proper stack traces and error handling
    • Reference: CLAUDE.md emphasizes using proper error handling patterns
  2. Type annotations (examples/kitchen-sink/src/backend/actors/websocket.ts:3):

    export function handleWebSocket(c: any, websocket: UniversalWebSocket)
    • Using any for the context parameter reduces type safety
    • Since this is a type-checking fix PR, consider using proper types (e.g., ActorContext or similar)
  3. AsyncAPI formatting changes (rivetkit-asyncapi/asyncapi.json):

    • The file shows 435 additions / 488 deletions, but these are purely whitespace changes (spaces → tabs)
    • This is fine, but ensure your formatting tools are consistent across the project

Potential Bugs 🔍

No critical bugs identified, but some observations:

  1. WebSocket parameter removal (examples/kitchen-sink/src/backend/actors/websocket.ts:3):

    • Removed opts parameter and changed opts.request.url to c.request.url
    • This looks correct based on the API change, where request moved to the context object
    • ✅ Properly updated in the function body (line 15)
  2. Lifecycle hook naming:

    • onStart/onStoponWake/onSleep better reflects the actor lifecycle semantics
    • The change is applied consistently across all files
    • ✅ Good naming that clarifies these are sleep/wake cycles, not create/destroy

Performance Considerations ⚡

No performance concerns. These are purely API signature and naming updates.

Security Concerns 🔒

Minor concern:

  • The null check for c.request (cursors-raw-websocket/src/backend/registry.ts:60) throws an error, which is good
  • However, ensure the error is handled gracefully at the WebSocket upgrade layer to avoid exposing internal errors to clients
  • The WebSocket is closed with proper code (1008) when sessionId is missing, which is correct

Test Coverage 🧪

Observations:

  • Test file updated (engine/tests/smoke/src/server/registry.ts) shows the API is tested
  • No new tests added, which is appropriate for a refactoring/type-fix PR
  • ✅ Existing test structure maintained

Recommendation:

  • Consider adding a test case for the c.request === undefined scenario in WebSocket handlers
  • This would ensure proper error handling for the new null check

Adherence to CLAUDE.md Standards ✅

Followed:

  • ✅ Uses kebab-case for naming conventions
  • ✅ Proper git commit message format: chore(rivetkit): fix type checks
  • ✅ No formatting changes committed (avoiding cargo fmt as instructed)

Could improve:

  • ⚠️ Error handling: Use Error objects instead of string throws (see point 1 above)
  • ⚠️ Type annotations: Avoid any types where possible

Summary of Recommendations

High Priority:

  1. Change throw "Missing request" to throw new Error("Missing request") for proper error handling

Medium Priority:
2. Replace any types with proper context types in WebSocket handlers
3. Add test coverage for the c.request null check scenario

Low Priority:
4. Verify AsyncAPI formatting consistency matches project standards

Overall Assessment

Rating: ✅ Approved with minor suggestions

This is a solid refactoring PR that addresses type checking issues and API consistency. The changes are minimal, focused, and maintain backward compatibility through clear naming improvements. The only concerns are minor code quality improvements around error handling and type safety.

Great work maintaining consistency across all examples and tests! 🎉

@NathanFlurry NathanFlurry force-pushed the 11-12-feat_rivetkit_add_ability_to_destroy_actors branch from d58f1db to fc21704 Compare November 13, 2025 22:35
@NathanFlurry NathanFlurry force-pushed the 11-12-chore_rivetkit_fix_type_checks branch from 94644ac to f3bbd90 Compare November 13, 2025 22:35
Base automatically changed from 11-12-feat_rivetkit_add_ability_to_destroy_actors to main November 13, 2025 22:38
@NathanFlurry NathanFlurry force-pushed the 11-12-chore_rivetkit_fix_type_checks branch from f3bbd90 to 8ce6318 Compare November 13, 2025 22:39
@NathanFlurry NathanFlurry merged commit cbc5142 into main Nov 13, 2025
6 of 10 checks passed
@NathanFlurry NathanFlurry deleted the 11-12-chore_rivetkit_fix_type_checks branch November 13, 2025 22:39
@claude
Copy link

claude bot commented Nov 13, 2025

Code Review for PR #3462: chore(rivetkit): fix type checks

I've reviewed this pull request and here's my feedback:

Summary

This PR appears to be a cleanup/refactoring that fixes TypeScript type issues across the RivetKit codebase. The changes include:

  • Renaming lifecycle hooks (onStartonWake, onStoponSleep, onFetchonRequest)
  • Fixing undefined request handling in WebSocket handlers
  • Removing deprecated transport option from frontend examples
  • Reformatting AsyncAPI schema
  • Improving error handling in the Node.js utilities

✅ Positive Observations

1. API Consistency Improvements

  • The lifecycle hook renames (onStartonWake, onStoponSleep) appear to be improving naming clarity
  • Consistent rename of onFetchonRequest aligns better with modern web standards

2. Better Null Safety

  • In examples/cursors-raw-websocket/src/backend/registry.ts:60-61, added proper null check:
    if (!c.request) throw "Missing request";
    const url = new URL(c.request.url);
    This prevents potential runtime errors.

3. Code Simplification

  • Removed unnecessary transport selection UI/logic from kitchen-sink example
  • Simplified WebSocket handler signatures by removing unused opts parameter
  • Request now properly accessed via c.request instead of being passed separately

4. JSON Formatting

  • The AsyncAPI schema reformatting improves readability with consistent indentation (tabs)

⚠️ Issues & Concerns

1. Error Handling Anti-Pattern (High Priority)

In examples/cursors-raw-websocket/src/backend/registry.ts:60:

if (!c.request) throw "Missing request";

Issue: Throwing a string instead of an Error object is an anti-pattern.

Recommendation: Use proper Error objects:

if (!c.request) throw new Error("Missing request");

This ensures proper stack traces and better debugging experience.


2. Error Handling in Node.js Utils (Medium Priority)

In rivetkit-typescript/packages/rivetkit/src/utils/node.ts:73-78:

catch (err) {
    console.warn(
        "Node.js modules not available, file system driver will not work",
        err,
    );
    throw err;
}

Issues:

  • Using console.warn before re-throwing makes this confusing - is it a warning or an error?
  • The warning message will appear even in expected scenarios (like running in browser/edge environments)
  • No structured logging using the tracing library mentioned in CLAUDE.md

Recommendation:

catch (err) {
    // Remove console.warn or use tracing library
    throw new Error(
        "Node.js modules not available. File system driver requires Node.js environment.",
        { cause: err }
    );
}

3. Breaking Changes Not Documented

The lifecycle hook renames (onStartonWake, onStoponSleep, onFetchonRequest) are breaking API changes but:

  • The PR title is chore instead of feat! or breaking
  • No migration guide or deprecation warnings
  • Examples are updated but external users would face breaking changes

Recommendation:

  • Consider if this should be marked as a breaking change
  • Add deprecation warnings in a prior release if this is a public API
  • Document the migration path

4. Missing Test Coverage

I don't see any test file changes in this PR, yet there are significant API changes:

  • No tests verifying the new onWake/onSleep hooks work correctly
  • No tests for the request null checking behavior
  • No tests for the WebSocket signature changes

Recommendation:

  • Add tests for the new lifecycle hooks
  • Add test cases for the request null check path
  • Verify existing tests still pass with the signature changes

5. Incomplete Signature Cleanup

In examples/kitchen-sink/src/backend/actors/websocket.ts:3-4:

export function handleWebSocket(c: any, websocket: UniversalWebSocket) {

Issue: Using any type defeats the purpose of TypeScript type checking.

Recommendation:

export function handleWebSocket(
    c: ActorContext<typeof demo>, 
    websocket: UniversalWebSocket
) {

6. AsyncAPI Schema Changes

The rivetkit-asyncapi/asyncapi.json file shows -488/+435 lines. This appears to be mostly formatting, but:

  • Hard to verify no semantic changes occurred
  • No explanation in PR description about why this was regenerated

Recommendation:

  • Confirm this is purely formatting
  • If regenerated, mention the tool/command used
  • Consider using a schema validator in CI

🔒 Security Considerations

1. WebSocket Connection Handling
The null check for c.request is good, but the error handling could be improved:

if (!sessionId) {
    websocket.close(1008, "Missing sessionId");
    return;
}

Using close code 1008 (Policy Violation) is appropriate. Good!

2. No Obvious Security Issues

  • No credential exposure
  • No SQL injection vectors introduced
  • No XSS vulnerabilities added

🚀 Performance Considerations

1. No Obvious Performance Regressions

  • The changes are mostly type-level and refactoring
  • Request handling path seems unchanged in terms of performance

2. Frontend Bundle Size
Removing the transport option UI reduces bundle size slightly (good!).


📋 Checklist for Author

Before merging, please address:

  • Change error string throw to Error object (high priority)
  • Reconsider console.warn + throw pattern in node.ts
  • Add tests for new lifecycle hooks and API changes
  • Fix any type usage in websocket.ts
  • Confirm AsyncAPI schema changes are intentional
  • Consider if this should be marked as a breaking change
  • Verify all existing tests pass
  • Document migration path if this is a breaking change

📝 Minor Suggestions

  1. Consider adding JSDoc comments for the new lifecycle hooks explaining when they're called
  2. The formatting in AsyncAPI went from spaces to tabs - ensure this matches project conventions
  3. Consider adding type guards for the request null check instead of throwing

Overall Assessment

Code Quality: Good overall structure, but needs refinement in error handling
Breaking Changes: Yes - API changes without proper versioning signals
Test Coverage: Insufficient - no tests for the API changes
Security: No new vulnerabilities introduced
Performance: No concerns

Recommendation: Request changes - primarily to fix error handling, add tests, and clarify breaking change status.


Review generated by Claude Code - please validate all suggestions before implementing.

@claude
Copy link

claude bot commented Nov 13, 2025

PR Review: Type Check Fixes

Summary

This PR addresses TypeScript type checking issues in the RivetKit codebase, primarily focusing on API renaming and improving type safety across examples and core packages.


Positive Aspects ✅

  1. API Consistency: The renaming from onFetch to onRequest (demo.ts:38, registry.ts in raw-fetch-handler) is more semantically accurate and aligns better with modern web standards.

  2. Lifecycle Hook Renaming: The changes from onStart/onStop to onWake/onSleep (smoke test registry.ts:7-12) better reflect the actor lifecycle semantics and are more intuitive.

  3. Type Safety Improvements:

    • Added null check for c.request in cursors-raw-websocket (registry.ts:60) prevents potential runtime errors
    • Removed unused parameter opts from WebSocket handler signature (websocket.ts:4) improves type correctness
  4. Code Cleanup:

    • Removed transport selection UI/logic from kitchen-sink example (ConnectionScreen.tsx, App.tsx) simplifies the API surface
    • AsyncAPI formatting change uses tabs consistently, following the project's hard-tab convention

Areas for Improvement 🔍

1. Error Handling - cursors-raw-websocket/registry.ts:60

if (!c.request) throw "Missing request";

Issue: Throwing a string instead of a proper Error object is an anti-pattern.

Recommendation:

if (!c.request) throw new Error("Missing request");

Or better yet, if this is part of RivetKit's error system mentioned in CLAUDE.md, use the custom error handling:

if (!c.request) throw new ActorRequestError().build();

2. Missing Context - cloudflare-workers/actor-driver.ts

The changes in this file look good, but I notice the comment on line 290 still says "Spawn onStop in background" when calling #callOnStopAsync. This is accurate since the method does call actor.onStop at line 300, but it would be helpful to verify this aligns with the onWake/onSleep naming convention changes seen in the examples.

Question: Should the internal onStop method be renamed to onSleep for consistency, or do they serve different purposes?

3. Type Safety - websocket.ts:4

export function handleWebSocket(c: any, websocket: UniversalWebSocket) {

Issue: Using any type defeats the purpose of TypeScript's type checking.

Recommendation: Define a proper type for the context parameter. For example:

import type { ActorContext } from 'rivetkit'; // or appropriate type
export function handleWebSocket(c: ActorContext<typeof demo>, websocket: UniversalWebSocket) {

4. AsyncAPI Schema Changes

The massive reformatting (435 additions, 488 deletions) makes it difficult to identify actual semantic changes.

Questions:

  • Were there any semantic changes to the schema, or is this purely formatting?
  • If purely formatting, was this an automated formatter run?
  • Consider using --ignore-whitespace in diffs for formatting-only changes

Performance Considerations ⚡

No significant performance concerns. The changes are primarily type-related and don't introduce algorithmic complexity or resource usage changes.


Security Concerns 🔒

Low Risk: No obvious security vulnerabilities introduced. The null check added in cursors-raw-websocket actually improves defensive programming.


Test Coverage 🧪

Concern: This PR updates test files (smoke/registry.ts) and example code but doesn't appear to include any new tests for the renamed APIs.

Recommendations:

  1. Verify existing tests pass with the renamed hooks (onWake/onSleep, onRequest)
  2. Consider adding tests that specifically validate the c.request null check behavior
  3. Add integration tests that verify the lifecycle hooks are called in the correct order

Minor Observations

  1. node.ts additions (lines 32-33): The @ts-ignore comment for import.meta.url is acceptable given the ESM/CommonJS dual support, but ensure this is documented for future maintainers.

  2. Consistency: The removal of transport options from the frontend is good for simplifying the API, but ensure this is documented in any user-facing changelog or migration guide.


Verdict

Recommendation: ✅ Approve with minor suggestions

The PR successfully addresses type checking issues and improves API consistency. The main concerns are:

  1. The string throw in cursors-raw-websocket (should be Error object)
  2. The any type in websocket.ts (should have proper typing)
  3. Need to verify test coverage for the renamed APIs

These are relatively minor issues that could be addressed in a follow-up PR if needed, but ideally should be fixed before merging for better code quality.


Questions for the Author

  1. Is the AsyncAPI formatting change intentional or a side effect of a formatter? If intentional, what prompted it?
  2. Should internal lifecycle methods (onStop) be renamed for consistency with the public onWake/onSleep naming?
  3. Have you run the full test suite to ensure all renamed APIs work correctly?

@claude
Copy link

claude bot commented Nov 13, 2025

Pull Request Review

Summary

This PR updates RivetKit to fix type checks by renaming lifecycle hooks and fixing type inconsistencies across the codebase. The changes align with the current API design documented in actor/config.ts.


✅ Positive Aspects

  1. Correct lifecycle hook naming: The changes properly rename onStartonWake and onStoponSleep, which aligns with the actual lifecycle hooks defined in the ActorConfigSchema (lines 47-48 of actor/config.ts).

  2. Consistent API updates: The renaming of onFetchonRequest is correct and matches the actual hook name defined in the schema (line 54).

  3. Better error handling: The addition of null checks in cursors-raw-websocket/src/backend/registry.ts:60 (if (!c.request) throw "Missing request") prevents potential runtime errors.

  4. AsyncAPI formatting: The rivetkit-asyncapi/asyncapi.json file has been properly reformatted with consistent indentation (tabs instead of spaces), improving readability.

  5. Cloudflare driver cleanup: The removal of the unused opts parameter in actor-driver.ts:291 and the WebSocket handler in websocket.ts properly reflects that c.request is now directly available on the context.


🔍 Issues and Concerns

1. Inconsistent Error Handling (Medium Priority)

Location: examples/cursors-raw-websocket/src/backend/registry.ts:60

if (!c.request) throw "Missing request";

Issue: Throwing a raw string instead of a proper Error object is not best practice. This makes error handling and stack traces less useful.

Recommendation:

if (!c.request) throw new Error("Missing request");

2. Type Safety Concern (Medium Priority)

Location: examples/cursors-raw-websocket/src/backend/registry.ts:60

The fact that c.request can be undefined suggests the WebSocketContext type might not be correctly constraining this property. According to actor/config.ts:418-428, the onRequest handler receives request: Request as a parameter, but onWebSocket receives it via the context.

Question: Should c.request always be available in the WebSocketContext? If so, the type definition should reflect this and the null check would be unnecessary.

3. Missing Import Fix (Low Priority)

Location: rivetkit-typescript/packages/rivetkit/src/utils/node.ts:1

While the addition of createRequire import is correct, the file shows good practices with detailed comments explaining the synchronous require usage. No issues here, just noting this is part of the type fixes.

4. Incomplete AsyncAPI Changes

Location: rivetkit-asyncapi/asyncapi.json

The diff was truncated (... [175 lines truncated] ...), making it impossible to fully review the AsyncAPI changes. Based on the visible changes, it appears to be primarily formatting (spaces → tabs), which is good for consistency.

Recommendation: Verify that no semantic changes were made to the API specification beyond formatting.


🧪 Testing Recommendations

  1. Type checking: Run TypeScript compilation across all affected packages to ensure no type errors remain:

    cd rivetkit-typescript && pnpm tsc --noEmit
  2. Lifecycle hook execution: Test that onWake and onSleep hooks are properly called during actor lifecycle events, especially in the smoke tests.

  3. WebSocket handling: Verify the WebSocket examples (cursors-raw-websocket, kitchen-sink) still work correctly with the c.request changes.

  4. HTTP request handling: Test the onRequest hook in examples like raw-fetch-handler to ensure the rename didn't break functionality.


📋 Code Quality Assessment

  • Consistency: ✅ Changes are consistent across the codebase
  • Type Safety: ⚠️ Minor concern with nullable c.request (see issue [SVC-2479] Send cluster events to PostHog #2)
  • Error Handling: ⚠️ String throw instead of Error object (see issue [SVC-2555] Set up issue templates #1)
  • Documentation: ✅ The lifecycle hooks in actor/config.ts have excellent documentation
  • Test Coverage: ❓ Unable to verify if tests were updated (no test changes in PR)

🔒 Security Considerations

No security concerns identified. The changes are primarily type corrections and API naming consistency.


⚡ Performance Considerations

No performance impact expected. These are compile-time type fixes and API renames without algorithmic changes.


📝 Suggested Improvements

  1. Fix the error throwing to use proper Error objects
  2. Consider making c.request non-nullable in WebSocketContext if it should always be present
  3. Add a test to verify the lifecycle hook renames work correctly
  4. Update any documentation that might reference the old hook names (onStart, onStop", onFetch`)

✨ Overall Assessment

Status: ✅ Approve with minor suggestions

This is a solid PR that fixes important type consistency issues in RivetKit. The lifecycle hook renames are necessary and correct. The minor issues identified are non-blocking but should be addressed for better code quality.

The changes properly align the implementation with the type definitions in ActorConfigSchema, which will prevent runtime type errors and improve developer experience.


Reviewed by: Claude Code
Review Date: 2025-11-13

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.

3 participants