Skip to content

Conversation

@NathanFlurry
Copy link
Member

No description provided.

Copy link
Member Author


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.

@railway-app
Copy link

railway-app bot commented Jan 27, 2026

🚅 Deployed to the rivet-pr-4052 environment in rivet-frontend

Service Status Web Updated (UTC)
frontend-cloud 😴 Sleeping (View Logs) Web Jan 27, 2026 at 11:05 pm
website 😴 Sleeping (View Logs) Web Jan 27, 2026 at 11:03 pm
frontend-inspector 😴 Sleeping (View Logs) Web Jan 27, 2026 at 11:02 pm
mcp-hub ✅ Success (View Logs) Web Jan 27, 2026 at 10:55 pm

@claude
Copy link

claude bot commented Jan 27, 2026

PR Review: Add noCreate to useActor

Summary

This PR adds a noCreate option to the useActor hook and related actor management functionality. When enabled, this option only retrieves existing actors without creating them, throwing an error if the actor doesn't exist.

Code Quality ✅

Strengths:

  • Clean implementation that follows existing code patterns
  • Proper documentation with JSDoc comments explaining the behavior
  • Consistent with the existing client.get() API

Observations:

  • The implementation correctly branches between client.get() and client.getOrCreate() based on the noCreate flag (lines 439-455)
  • The noCreate option is properly included in the hash function (line 517), which is critical for cache key uniqueness

Potential Issues 🟡

1. Error Handling Behavior
The documentation states "Throws an error if the actor is not found" but looking at the implementation:

  • Line 506-514: The try-catch block catches errors and sets connStatus: "disconnected" with the error stored in state
  • This means the error is caught and stored rather than thrown to the caller
  • Impact: Users calling useActor with noCreate: true won't get a synchronous exception but will need to check the error state

Recommendation: Either:

  1. Update the documentation to clarify that errors are captured in the actor state rather than thrown, or
  2. Consider if noCreate errors should be handled differently (though the current approach is consistent with other connection errors)

2. Missing createInRegion and createWithInput in Hash Function
The hash function now includes noCreate but still includes other creation-related options:

  • Line 517: JSON.stringify({ name, key, params, noCreate })
  • However, createInRegion and createWithInput are NOT included in the hash

Concern: Two useActor calls with the same name/key/params but different createInRegion or createWithInput values would share the same cache entry. This could lead to unexpected behavior where the first call's creation options are used for all subsequent calls.

Example:

// Component A
useActor({ name: 'chat', key: 'room1', createInRegion: 'us-west' })

// Component B - will reuse the same actor instance from A
useActor({ name: 'chat', key: 'room1', createInRegion: 'eu-central' })

Recommendation: Consider whether createInRegion and createWithInput should be part of the hash. If these options should distinguish between different actor instances, they should be included. If they're only relevant for the first creation and subsequent calls should reuse the same instance regardless of these options, the current behavior is correct (but should be documented).

Performance Considerations ✅

  • No performance concerns
  • The conditional logic adds negligible overhead
  • The hash function remains efficient

Security Concerns ✅

  • No security issues identified
  • The implementation properly delegates to the existing client.get() method which should have its own security checks

Test Coverage ❌

Missing:

  • No test files found in rivetkit-typescript/packages/framework-base/
  • This feature should have tests covering:
    1. noCreate: true with existing actor (should succeed)
    2. noCreate: true with non-existent actor (should error)
    3. noCreate: false or undefined (default behavior)
    4. Verify that noCreate is included in the hash for cache uniqueness

Recommendation: Add test coverage for this new option, especially since error handling behavior is critical for the noCreate use case.

Additional Observations

Good Practices:

  • The option defaults to false, maintaining backward compatibility
  • Documentation is clear and consistent across both interface definitions
  • The implementation properly passes only relevant options to each method (params for get, additional create options for getOrCreate)

Naming:

  • The noCreate naming is consistent with a negative boolean flag pattern, which is clear in this context

Summary

This is a solid implementation that adds useful functionality. The main concerns are:

  1. Documentation clarity around error handling behavior
  2. Potential cache key issue with createInRegion/createWithInput not being in the hash
  3. Missing test coverage for the new feature

The first two issues should be addressed before merging. The test coverage should ideally be added but may be acceptable to defer if there's a plan to add comprehensive testing later.


Review generated for commit 4aa0e78

@pkg-pr-new
Copy link

pkg-pr-new bot commented Jan 27, 2026

More templates

@rivetkit/cloudflare-workers

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

@rivetkit/db

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

@rivetkit/framework-base

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

@rivetkit/next-js

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

@rivetkit/react

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

rivetkit

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

@rivetkit/sql-loader

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

@rivetkit/virtual-websocket

pnpm add https://pkg.pr.new/rivet-dev/rivet/@rivetkit/virtual-websocket@4052

@rivetkit/engine-runner

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

@rivetkit/engine-runner-protocol

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

commit: 4aa0e78

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