Skip to content

Conversation

@NathanFlurry
Copy link
Member

No description provided.

Copy link
Member Author

NathanFlurry commented Jan 24, 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.

@NathanFlurry NathanFlurry changed the title queues example workflow example Jan 24, 2026
// Workflow Sandbox - Actor Registry
// Each actor demonstrates a different workflow feature using actor-per-workflow pattern

import { setup } from "rivetkit";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The 'setup' function is imported from 'rivetkit' but the module cannot be found. This is because the root package.json needs to have resolutions for RivetKit packages. Add 'rivetkit': 'workspace:*' to the resolutions in the root package.json.

Spotted by Graphite Agent (based on CI logs)

Fix in Graphite


Is this helpful? React 👍 or 👎 to let us know.

@claude
Copy link

claude bot commented Jan 24, 2026

Pull Request Review: Workflow Example

This PR adds a comprehensive workflow sandbox example and addresses several friction points in the RivetKit workflow API. Overall, this is a valuable contribution that improves developer experience and provides excellent documentation through a working example.


✅ Strengths

1. Excellent Documentation

  • workflow-friction.md is exceptional - documenting pain points during development is invaluable for improving the API
  • The example README follows the template correctly with clear feature sections
  • Good inline comments explaining workflow patterns

2. API Improvements

The PR addresses real friction points:

  • Added missing send() method to ActorQueue
  • Added missing broadcast() method to ActorWorkflowContext
  • Re-exported Loop from rivetkit/workflow to fix package resolution
  • Updated CLAUDE.md with RivetKit package resolution guidelines

3. Comprehensive Example

The workflow-sandbox example demonstrates:

  • Sequential steps (order processing)
  • Sleep/timers (countdown)
  • Loops with state (batch processing)
  • Listen with timeout (approval queue)
  • Join (parallel data fetching)
  • Race (work vs timeout)
  • Rollback/compensating transactions (payment)

Each pattern is implemented as a separate actor with clean separation of concerns.


⚠️ Issues & Recommendations

1. Type System Workaround (Critical)

Issue: The actorCtx() helper in _helpers.ts is a workaround for a fundamental type system problem documented in workflow-friction.md.

Location: examples/workflow-sandbox/src/actors/_helpers.ts:10-12

Problem: Every example needs to cast the loop context to access state and broadcast, which defeats type safety.

Recommendation:

  • Short-term: This workaround is acceptable for the example, but add a TODO comment linking to a tracking issue
  • Long-term: Fix the root cause by making ActorWorkflowContext the declared type for loop callbacks (as noted in friction log)

2. Type Cast in workflow/mod.ts

Issue: The return type is cast to satisfy RunConfig.

Recommendation: Consider:

  1. Adjusting the RunConfig type to accept the actual return type
  2. Ensuring the workflow run function signature matches what RunConfig expects
  3. Adding a comment explaining why the cast is necessary if it's intentional

3. Error Handling Patterns

Issue: All actor files use try-catch with error serialization, but there's no consistent error handling pattern.

Examples:

  • order.ts:85-96 - catches and sets state.error
  • payment.ts:158-170 - same pattern
  • No validation of error types or structured error objects

Recommendation:

  • Consider creating a shared error handling utility
  • Define error types instead of using Error | unknown
  • Follow the custom error system documented in CLAUDE.md (packages/common/error/)

4. Logging Style Inconsistency

Issue: Pick a consistent naming convention for IDs (orderId vs txId vs id)

5. Documentation Completeness

Issue: The workflow-friction.md document is excellent but lives at the repo root instead of in a docs/ directory.

Recommendation:

  • Move to docs/development/workflow-friction.md or similar
  • Add a link from the main README or CONTRIBUTING.md
  • Consider converting this into tracking issues for each friction point

🔍 Security Considerations

✅ No security concerns identified:

  • No sensitive data handling
  • No external API calls
  • Simulated payment/order processing only

📊 Test Coverage

Missing: No test files in tests/ directory for the workflow-sandbox example.

Recommendation: Add tests following the pattern from examples/CLAUDE.md


📝 Code Quality

Positive:

  • Consistent formatting
  • TypeScript strict mode enabled
  • Good separation of concerns
  • Clear naming conventions

Areas for Improvement:

  • Reduce duplication in actor patterns (consider shared utilities)
  • Add JSDoc comments for exported types

📋 Checklist

  • ✅ Follows repository conventions (CLAUDE.md)
  • ✅ Uses pnpm workspace patterns correctly
  • ✅ ESM imports with .ts extensions
  • ✅ Structured logging with tracing
  • ⚠️ Type safety (workarounds present)
  • ❌ Missing test coverage
  • ⚠️ Friction points documented but not fully resolved

🎬 Conclusion

Recommendation: Approve with minor changes

This PR significantly improves the RivetKit developer experience by:

  1. Adding missing API methods that were clearly needed
  2. Providing a comprehensive workflow example
  3. Documenting friction points for future improvement

The type system workarounds are acceptable for now, given they're well-documented in the friction log. However, these should be addressed in follow-up work.

Required before merge:

  • None (approval can proceed)

Recommended follow-ups:

  1. Create tracking issues for each friction point in workflow-friction.md
  2. Add test coverage for the workflow-sandbox example
  3. Fix the type system issues so actorCtx helper isn't needed
  4. Review and potentially refactor the 880-line App.tsx component

Great work on documenting the friction points - this kind of developer feedback is invaluable! 🎉

Comment on lines +44 to +50
import { timer } from "./actors/timer.ts";
import { order } from "./actors/order.ts";
import { batch } from "./actors/batch.ts";
import { approval } from "./actors/approval.ts";
import { dashboard } from "./actors/dashboard.ts";
import { race } from "./actors/race.ts";
import { payment } from "./actors/payment.ts";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These imports need to be sorted alphabetically to pass Biome linting. Run 'pnpm biome check --apply' to automatically fix this issue.

Spotted by Graphite Agent (based on CI logs)

Fix in Graphite


Is this helpful? React 👍 or 👎 to let us know.

Comment on lines +134 to +136
useEffect(() => {
actor.connection?.getOrder().then(setOrder);
}, [actor.connection]);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This creates an unhandled floating Promise. Add a .catch() handler to properly handle potential errors: actor.connection?.getOrder().then(setOrder).catch(error => console.error('Failed to get order:', error));

Spotted by Graphite Agent (based on CI logs)

Fix in Graphite


Is this helpful? React 👍 or 👎 to let us know.

Comment on lines +714 to +716
useEffect(() => {
actor.connection?.getTask().then(setTask);
}, [actor.connection]);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add error handling to this Promise: actor.connection?.getTask().then(setTask).catch(error => console.error('Failed to get task:', error));

Spotted by Graphite Agent (based on CI logs)

Fix in Graphite


Is this helpful? React 👍 or 👎 to let us know.

Comment on lines +527 to +529
<button className="approve" onClick={() => onApprove("Admin")}>
Approve
</button>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If onApprove returns a Promise, handle it properly: onClick={() => { const result = onApprove('Admin'); if (result instanceof Promise) result.catch(err => console.error('Approval failed:', err)); }}

Spotted by Graphite Agent (based on CI logs)

Fix in Graphite


Is this helpful? React 👍 or 👎 to let us know.

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