Skip to content

Conversation

@NathanFlurry
Copy link
Member

No description provided.

@railway-app
Copy link

railway-app bot commented Jan 22, 2026

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

Service Status Web Updated (UTC)
website 😴 Sleeping (View Logs) Web Jan 22, 2026 at 7:24 am
frontend-cloud ❌ Build Failed (View Logs) Web Jan 22, 2026 at 7:15 am
frontend-inspector ❌ Build Failed (View Logs) Web Jan 22, 2026 at 7:15 am
mcp-hub ❌ Build Failed (View Logs) Web Jan 22, 2026 at 7:14 am

Copy link
Member Author

NathanFlurry commented Jan 22, 2026

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 Jan 22, 2026

Code Review: Effect Integration for RivetKit

This PR adds a comprehensive Effect.js integration example for RivetKit actors. Overall, the implementation is well-structured and demonstrates good patterns for integrating Effect with RivetKit. Below are my observations and recommendations.


Positive Aspects

Well-structured architecture - Clean separation between Effect wrappers (action.ts, actor.ts, hooks.ts) and example actors
Comprehensive coverage - Covers actions, workflows, and all lifecycle hooks
Good test coverage - Tests cover key scenarios including workflows, lifecycle hooks, and state management
Type safety - Good use of TypeScript generics to preserve type information through Effect wrappers


Issues & Concerns

🔴 Critical: Code Duplication in actor.ts

Location: examples/effect/src/effect/actor.ts:251-335

The effect() and workflow() functions in actor.ts are identical duplicates of those in action.ts. This creates maintenance issues.

Recommendation: Remove these duplicate functions from actor.ts and import them from action.ts instead:

// In actor.ts - remove lines 251-335 entirely
// Users should import from action.ts instead

🟡 Medium: Inconsistent Error Handling

Location: examples/effect/src/effect/actor.ts:286-288 and action.ts:103-104

Both effect() and workflow() functions call waitUntil() on the promise, but only workflow() should do this for durability. The effect() function in actor.ts appears to incorrectly include this behavior.

In action.ts:

  • effect() (lines 22-62) correctly does NOT call waitUntil
  • workflow() (lines 64-108) correctly DOES call waitUntil

In actor.ts:

  • effect() (lines 251-292) incorrectly calls waitUntil on line 288
  • workflow() (lines 294-335) correctly calls waitUntil

Impact: This may cause regular actions to be treated as durable workflows, potentially affecting performance and execution semantics.

Recommendation: If keeping separate effect() functions in both files, ensure actor.ts:effect() does NOT call waitUntil to match the behavior in action.ts. Better yet, consolidate to avoid duplication (see above).

🟡 Medium: Type Safety Concerns with any Casts

Locations:

  • action.ts:58: as Effect.Effect<AEff, never, never>
  • actor.ts:284: as Effect.Effect<AEff, any, never>
  • Multiple hooks in hooks.ts with c as any casts

Issue: The type assertions using any bypass TypeScript's type checking and could hide type mismatches.

Recommendation:

  • Use proper type narrowing or branded types instead of any casts
  • If type assertions are unavoidable, add comments explaining why they're safe
  • Consider using more specific error types in the Effect type signatures

🟡 Medium: README Doesn't Follow Template

Location: examples/effect/README.md

The README is missing:

  • Source code links in the "Implementation" section should use full GitHub URLs
  • Current: [src/effect/action.ts](https://github.com/rivet-dev/rivet/tree/main/examples/effect/src/effect/action.ts)
  • Should reference: src/actors.tssrc/actors/index.ts or individual actor files

According to .claude/resources/EXAMPLE_TEMPLATE.md, the "Implementation" section should include GitHub links to key source files.

Current Implementation section:

Key files:
- [`src/effect/action.ts`](https://github.com/rivet-dev/rivet/tree/main/examples/effect/src/effect/action.ts)
- [`src/effect/actor.ts`](https://github.com/rivet-dev/rivet/tree/main/examples/effect/src/effect/actor.ts)
- [`src/actors.ts`](https://github.com/rivet-dev/rivet/tree/main/examples/effect/src/actors.ts)

Recommendation:

  • Fix the path: src/actors.ts doesn't exist, should be src/actors/index.ts or point to specific actors like src/actors/counter.ts
  • Add more explanation about what each file does
  • Consider linking to the most illustrative examples (e.g., lifecycle-demo.ts, user.ts)

🟢 Minor: Missing .gitignore Entry

Location: examples/effect/.gitignore

The file is missing a newline at the end (line 2). While this works, it's a common convention to end files with a newline.

Recommendation: Add a newline at the end of the file.

🟢 Minor: Unused Type Alias Workaround

Locations: Multiple files use this pattern:

// Local type alias to work around AnyDatabaseProvider not being exported
type AnyDB = undefined;

Issue: The comment suggests this is a workaround, but the implementation just uses undefined. This might not actually work correctly if AnyDatabaseProvider is meant to support actual database providers.

Recommendation:

  • If this is intentional, clarify in the comment that undefined represents "no database provider"
  • If RivetKit should export AnyDatabaseProvider, file an issue to export it properly
  • Consider if this limits the example's utility for users who need database support

Security Concerns

🔒 No significant security issues identified. The example code doesn't handle sensitive data or external inputs in unsafe ways.

The validateEmail function in user.ts:20-22 is a stub that always returns true, but this is acceptable for example code.


Performance Considerations

Potential concern with waitUntil usage:

The waitUntil pattern in workflow() is good for durability, but the duplicate implementation in actor.ts:effect() could cause unintended performance implications if regular actions are inadvertently made durable.

Recommendation: Clearly document when to use effect() vs workflow(), and ensure the implementations are distinct.


Testing

Good test coverage for the core functionality
🟡 Suggestions:

  • Add a test that verifies workflows actually complete after actor termination (to validate waitUntil behavior)
  • Test error handling in workflows (e.g., what happens when InvalidEmailError is thrown)
  • Add tests for the Effect service injection mechanism

Recommendations Summary

Must Fix (Before Merge)

  1. Remove duplicate effect() and workflow() functions from actor.ts - This is code duplication that will cause maintenance issues
  2. Fix actor.ts:effect() to NOT call waitUntil - Current behavior is inconsistent with action.ts
  3. Fix README path - src/actors.ts should be src/actors/index.ts or link to specific actor files

Should Fix (Nice to Have)

  1. Improve type safety by reducing any casts
  2. Add more detailed implementation explanation in README
  3. Add tests for error handling and workflow durability
  4. Clarify the AnyDB type alias workaround

Nice to Have

  1. Add newline at end of .gitignore
  2. Add comments explaining unavoidable type assertions

Overall Assessment

This is a solid example that demonstrates good patterns for integrating Effect with RivetKit. The main concerns are around code duplication and ensuring consistent behavior between the effect() and workflow() implementations. Once the critical issues are addressed, this will be a valuable resource for users wanting to use Effect with RivetKit.

Recommended Action: Request changes to address the critical issues before merging.

@NathanFlurry NathanFlurry marked this pull request as ready for review January 22, 2026 08:12
@NathanFlurry NathanFlurry changed the base branch from 01-21-feat_rivetkit_add_actor_run_handler to graphite-base/4008 January 22, 2026 17: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