workflow example#4032
Conversation
|
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.
How to use the Graphite Merge QueueAdd 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. |
| // Workflow Sandbox - Actor Registry | ||
| // Each actor demonstrates a different workflow feature using actor-per-workflow pattern | ||
|
|
||
| import { setup } from "rivetkit"; |
There was a problem hiding this comment.
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)
Is this helpful? React 👍 or 👎 to let us know.
Pull Request Review: Workflow ExampleThis 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. ✅ Strengths1. Excellent Documentation
2. API ImprovementsThe PR addresses real friction points:
3. Comprehensive ExampleThe workflow-sandbox example demonstrates:
Each pattern is implemented as a separate actor with clean separation of concerns.
|
| 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"; |
There was a problem hiding this comment.
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)
Is this helpful? React 👍 or 👎 to let us know.
| useEffect(() => { | ||
| actor.connection?.getOrder().then(setOrder); | ||
| }, [actor.connection]); |
There was a problem hiding this comment.
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)
Is this helpful? React 👍 or 👎 to let us know.
| useEffect(() => { | ||
| actor.connection?.getTask().then(setTask); | ||
| }, [actor.connection]); |
There was a problem hiding this comment.
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)
Is this helpful? React 👍 or 👎 to let us know.
| <button className="approve" onClick={() => onApprove("Admin")}> | ||
| Approve | ||
| </button> |
There was a problem hiding this comment.
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)
Is this helpful? React 👍 or 👎 to let us know.
2bdce1c to
389f28e
Compare
7404ebe to
32bed1a
Compare
PR Review: Workflow ExampleThis PR adds comprehensive workflow sandbox examples and makes important improvements to the workflow engine. Overall, the changes are well-structured and demonstrate proper use of the workflow system. Here are my findings: ✅ Strengths
🐛 Potential Issues1. Indentation Inconsistency in index.ts (Line 878-879)Lines 878-879 in // Unrecoverable error
storage.error = extractErrorInfo(error);
storage.state = "rolling_back"; // Extra tab here
await flush(storage, driver); // Extra tab hereThis should be: storage.error = extractErrorInfo(error);
storage.state = "rolling_back";
await flush(storage, driver);2. Error Handling in executeWithTimeout (Line 487-502)The
Recommendation: Document this more prominently in the StepConfig type definition, not just in an internal method comment. Users need to understand that timeouts don't cancel work. 3. Rollback Handler Registration During Rollback ModeIn
The code seems correct (it only registers if 4. Race Condition in executeLiveWorkflowLines 516-534 in 5. Loop Iteration Cleanup Edge CaseIn 📝 Code Quality Observations1. File OrganizationThe workflow examples follow good structure with clear separation of concerns (actors, server, frontend). However, some actors (like 2. Error Types ConsistencyThe error classes in
This is fine, but consider documenting the philosophy: "Include metadata when it helps debugging, omit when error message is self-contained." 3. Magic NumbersSeveral constants are used directly in code:
4. Type Safety in executeListenNUntilImplThe if (limit <= 0) {
throw new Error("limit must be positive");
}🔒 Security Considerations
⚡ Performance Considerations
📚 Documentation Needs
🧪 Testing ObservationsThe test addition in
✨ Suggested Improvements
📋 Minor Nitpicks
SummaryThis is solid work that adds valuable functionality and documentation. The workflow engine changes demonstrate good understanding of distributed systems challenges (persistence, replay, partial failure). The examples are comprehensive and will help users learn the system. Primary concerns:
The code is production-ready after addressing the indentation issue. Great job overall! 🎉 |
389f28e to
3c7fabc
Compare
3c7fabc to
7dc3fdd
Compare
32bed1a to
2491e17
Compare
7dc3fdd to
0232d12
Compare
…WithTimeout When listenWithTimeout/listenUntil was used, workflows would only wake when the deadline passed, not when a message arrived. This broke approval workflows and other human-in-the-loop patterns. Changes: - Extended SleepError to optionally carry messageNames array - Modified listen methods to pass message names when throwing SleepError - Updated execution loop to wait for EITHER messages OR deadline - Fixed race branches to properly yield on SleepError instead of failing Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
0232d12 to
9cedde3
Compare
2491e17 to
889509a
Compare
Merge activity
|

No description provided.