Skip to content

Conversation

@NathanFlurry
Copy link
Member

No description provided.

@vercel
Copy link

vercel bot commented Dec 12, 2025

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

Project Deployment Preview Comments Updated (UTC)
rivetkit-serverless Error Error Dec 12, 2025 0:53am
3 Skipped Deployments
Project Deployment Preview Comments Updated (UTC)
rivet-cloud Ignored Ignored Dec 12, 2025 0:53am
rivet-inspector Ignored Ignored Preview Dec 12, 2025 0:53am
rivet-site Ignored Ignored Preview Dec 12, 2025 0:53am

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.

@claude
Copy link

claude bot commented Dec 12, 2025

Pull Request Review: Effect Example

Overview

This PR adds a new Effect.ts integration example for RivetKit, demonstrating how to use the Effect library with Rivet Actors. The implementation includes wrapper utilities for Effect integration and example actors showcasing the pattern.


🔴 Critical Issues

1. README Does Not Follow Template (examples/effect/README.md)

The README violates the documented template format from .claude/resources/EXAMPLE_TEMPLATE.md:

Missing Required Sections:

  • ## Features - Should highlight 3-5 RivetKit concepts demonstrated
  • ## Implementation - Should explain technical details with GitHub source links
  • ## Resources - Should link to relevant RivetKit documentation

Current Issues:

  • Title says "Counter for RivetKit" but this is an "Effect" example
  • Installation path is wrong: cd rivetkit/examples/counter should be cd rivetkit/examples/effect
  • Uses npm instead of pnpm (violates CLAUDE.md: "Use pnpm for all npm-related commands")
  • Prerequisites section includes "Node.js" which should be omitted per template guidelines

Required Fix:

# Effect.ts Integration for RivetKit

Example project demonstrating Effect.ts integration with RivetKit actors for type-safe, composable workflows.

[Learn More →](https://github.com/rivet-dev/rivetkit)

[Discord](https://rivet.dev/discord)[Documentation](https://rivetkit.org)[Issues](https://github.com/rivet-dev/rivetkit/issues)

## Getting Started

```sh
git clone https://github.com/rivet-dev/rivetkit
cd rivetkit/examples/effect
pnpm install
pnpm run dev

Run the connect script to interact with actors:

pnpm run connect

Features

  • Effect.ts generator-based actions with type-safe error handling
  • Durable workflow execution using @effect/workflow activities
  • Structured logging with Effect context integration
  • Concurrency control for parallel and serial action execution
  • State management with Effect.sync and Effect.promise wrappers

Implementation

This example provides Effect.ts wrappers around RivetKit's actor system, enabling developers to write actions using Effect generators.

Key implementation files:

The Action.effect() wrapper converts Effect generators into RivetKit actions, while Action.workflow() enables durable execution using waitUntil for background processing.

Resources

Read more about:

License

Apache 2.0


#### 2. **Commented Out Code** (examples/effect/src/actors/counter.ts:21-30)
There's a large block of commented-out code that should either be removed or uncommented if it's meant to demonstrate an alternative pattern:

```typescript
// increment: Action.createWithSchema(
// 	IncrementInput,
// 	function* (c, { amount }) {
// 		yield* Actor.updateState(c, (s) => {
// 			s.count += amount;
// 		});
// 		const s = yield* Actor.state(c);
// 		return s.count;
// 	},
// ),

Recommendation: Remove this code or add a clear comment explaining why it's kept as reference.

3. Unused Function (examples/effect/src/actors/user.ts:58-62, 73-77)

The createStripeCustomer and sendResendWelcomeEmail functions are defined but never called:

function createStripeCustomer(email: string): Effect.Effect<string> { /* ... */ }
function sendResendWelcomeEmail(email: string): Effect.Effect<void> { /* ... */ }

Recommendation: Either use these functions or remove them to keep the example focused.

4. Stub Email Validation (examples/effect/src/actors/user.ts:53-55)

The email validation always returns true:

function validateEmail(email: string): boolean {
	return true;
}

Recommendation: Either implement basic validation or add a comment explaining this is intentionally stubbed for demonstration purposes.


⚠️ Moderate Issues

5. Potential Type Safety Issue (examples/effect/src/effect/actor.ts:26)

The context() function uses a type assertion that may break type safety:

export const context = <...>(): Effect.Effect<...> => ActorContextTag as any;

Recommendation: This should use Effect.Tag properly or document why the cast is necessary.

6. Duplicate Effect/Workflow Logic (examples/effect/src/effect/actor.ts:252-336)

The effect() and workflow() functions at lines 252-336 appear to be duplicates of those in action.ts. This creates maintenance burden.

Recommendation: Consider if these should be exported from a single location or if the duplication serves a specific purpose that should be documented.

7. Error Handling in Workflow (examples/effect/src/actors/user.ts:16-21)

The updateEmail workflow returns an error but it's unclear how RivetKit handles Effect errors vs exceptions:

if (!validateEmail(newEmail)) {
	return yield* Effect.fail(new InvalidEmailError({ email: newEmail }));
}

Recommendation: Add documentation or tests showing how Effect errors propagate to RivetKit clients.

8. Missing Concurrency Manager Export (rivetkit-typescript/packages/rivetkit/src/actor/mod.ts:3)

The new ConcurrencyManager is used internally but might be useful to expose:

export * from "./instance/mod";

Recommendation: Consider if ConcurrencyManager should be part of the public API.


Positive Aspects

  1. Clean Separation of Concerns: The Effect integration is well-separated into src/effect/ directory
  2. Comprehensive Effect Utilities: Good coverage of state, vars, broadcast, logging, etc.
  3. Real-world Example: The user actor demonstrates practical workflow patterns with external service calls
  4. Proper Concurrency Handling: The new ConcurrencyManager implementation is well-structured with clear documentation
  5. Type Safety: Good use of TypeScript generics throughout the Effect wrappers

📝 Minor Issues / Suggestions

9. Inconsistent Import Path (examples/effect/src/actors/user.ts:3)

Uses @rivetkit/effect alias but counter.ts uses relative import:

import { Action } from "@rivetkit/effect";  // user.ts
import { Action } from "../effect";          // counter.ts

Recommendation: Be consistent - either use the alias everywhere or use relative imports.

10. Package.json Scripts (examples/effect/package.json:6-10)

Missing common development scripts:

Recommendation: Consider adding:

"lint": "tsc --noEmit",
"format": "prettier --write ."

11. Missing .gitignore Entry (examples/effect/.gitignore)

Missing newline at end of file (lines 1-2).

Recommendation: Add newline for POSIX compliance.


🔒 Security Considerations

✅ No security concerns identified. The example uses stub implementations for external services which is appropriate.


🧪 Test Coverage

⚠️ No tests found for the new Effect integration.

Recommendation: Add tests covering:

  1. Effect action execution
  2. Workflow durability with waitUntil
  3. Error handling with Effect.fail
  4. State mutations within Effect contexts
  5. Activity execution in workflows

📊 Performance Considerations

12. Multiple Effect.runPromise Calls

Both effect() and workflow() functions call Effect.runPromise() which could have performance implications:

const workflowPromise = Effect.runPromise(withContext);
c.waitUntil(workflowPromise.then(() => {}));

Recommendation: Consider documenting the performance characteristics or add benchmarks comparing Effect actions vs regular actions.


🎯 Summary

Must Fix Before Merge:

  1. ✅ Update README to follow template format
  2. ✅ Fix installation path and use pnpm instead of npm
  3. ✅ Remove or document commented-out code
  4. ✅ Remove unused functions or integrate them into the example

Should Fix:
5. Fix type safety in context() function
6. Document or consolidate duplicate effect/workflow implementations
7. Add tests for Effect integration
8. Consistent import paths across example files

Nice to Have:
9. Add development scripts to package.json
10. Add performance documentation
11. Fix .gitignore newline


Verdict

This is a valuable addition demonstrating Effect.ts integration with RivetKit. However, the README must be updated to follow the documented template before merging, and the commented/unused code should be cleaned up. The core implementation is solid, but would benefit from test coverage.

Recommendation: Request changes for critical issues, approve after fixes.

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