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 5:13pm
3 Skipped Deployments
Project Deployment Preview Comments Updated (UTC)
rivet-cloud Ignored Ignored Nov 13, 2025 5:13pm
rivet-inspector Ignored Ignored Preview Nov 13, 2025 5:13pm
rivet-site Ignored Ignored Preview Nov 13, 2025 5:13pm

Copy link
Member Author

NathanFlurry commented Nov 13, 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 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! 🎉

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