Allow fluent syntax for defining workflows#230
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughRefactors workflow definitions to a chained Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant DefineBuilder as "defineWorkflow(component, config)"
participant BoundWorkflow as "BoundWorkflow / RegisteredMutation"
participant ComponentWorkflow as "components.workflow"
participant ComponentEvent as "components.event"
Client->>DefineBuilder: defineWorkflow(component, config)
DefineBuilder-->>BoundWorkflow: .handler(fn) / .withHandlerRef(ref)
Client->>BoundWorkflow: start(ctx, args, options?)
BoundWorkflow->>ComponentWorkflow: create({ name, handle, workflowArgs: args })
ComponentWorkflow-->>BoundWorkflow: workflowId
BoundWorkflow->>ComponentEvent: createEvent(ctx, component, { workflowId, name, value })
ComponentEvent-->>BoundWorkflow: eventId
BoundWorkflow-->>Client: workflowId
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
ianmacartney
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
example/convex/example.ts (1)
9-21:⚠️ Potential issue | 🟡 MinorCarry forward the workflow-level retry default here.
This rewrite dropped
workpoolOptions.retryActionsByDefault, but the fluent API still wires per-workflow options throughregistered.workpoolOptions, which are merged and applied downstream. Without this setting, the firstgetGeocodingcall andgetWeathernow fall back to the global default (retryActionsByDefault: false), which is a behavior change unrelated to the API migration.Suggested fix
export const myWorkflow = defineWorkflow(components.workflow, { args: { location: v.string(), }, // If you also want to run runtime validation on the return value. returns: v.object({ name: v.string(), celsius: v.number(), farenheit: v.number(), windSpeed: v.number(), windGust: v.number(), }), + workpoolOptions: { + retryActionsByDefault: true, + }, }).bind(internal.example.exampleWorkflow);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@example/convex/example.ts` around lines 9 - 21, The workflow-level default for retries was dropped; update the defineWorkflow call for myWorkflow (the defineWorkflow(...).bind(internal.example.exampleWorkflow) block) to include workpoolOptions with retryActionsByDefault set to the previous default so registered.workpoolOptions retains and merges that flag (e.g., add workpoolOptions: { retryActionsByDefault: true } inside the defineWorkflow options) ensuring getGeocoding and getWeather inherit the workflow-level retry default.src/client/workflowMutation.ts (1)
29-38:⚠️ Potential issue | 🟠 MajorUse a dedicated validator for the bound
{ args: ... }invocation path.The condition
"args" in argsthrows a TypeError ifargsis null, undefined, or a primitive type (string, number, etc.). When validation fails andboundFnis set, this error is thrown before reachingINVALID_WORKFLOW_MESSAGE. Additionally, relying on theinoperator is less precise than explicit validation—any object with anargskey passes the check, even if the payload is otherwise malformed.Suggested fix
const workflowArgs = v.union( v.object({ workflowId: vWorkflowId, generationNumber: v.number(), }), v.object({ fn: v.string(), args: v.any(), }), ); +const boundWorkflowArgs = v.object({ + args: v.any(), +}); const INVALID_WORKFLOW_MESSAGE = `Invalid arguments for workflow: Did you invoke the workflow with ctx.runMutation() instead of workflow.start()? Pro tip: to start a workflow directly from the CLI or dashboard, you can use args '{ fn: "path/to/file:workflowName", args: { ...your workflow args } }'`; @@ - if (!validate(workflowArgs, args) && boundFn && "args" in args) { + if (boundFn && validate(boundWorkflowArgs, args)) { // Bound workflow called directly with { args: ... } const fn = makeFunctionReference(boundFn); const workflowId = await ctx.runMutation(component.workflow.create, { workflowName: safeFunctionName(fn), workflowHandle: await createFunctionHandle(fn), workflowArgs: args.args, maxParallelism: workpoolOptions.maxParallelism, }); return workflowId; }
🧹 Nitpick comments (1)
src/client/index.ts (1)
235-327: Consider extracting shared logic betweenBoundWorkflowandWorkflowManager.The
BoundWorkflowmethods (start,status,cancel,restart,cleanup,sendEvent,createEvent) largely duplicate the correspondingWorkflowManagermethods (lines 421-705). While the duplication is understandable for providing two parallel API surfaces, extracting the core logic into shared helper functions would reduce maintenance burden and ensure behavior stays consistent.For example, a shared
createWorkflowStart(component, ref, args, options)helper could be used by both APIs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/index.ts` around lines 235 - 327, BoundWorkflow methods (start, status, cancel, restart, cleanup, sendEvent, createEvent) duplicate logic from WorkflowManager; factor out shared helpers and call them from both surfaces. Implement small pure helpers such as createWorkflowStart(component, ref, args, options) that encapsulates createFunctionHandle, safeFunctionName and the ctx.runMutation call to component.workflow.create (returning workflowId), getWorkflowStatus(ctx, component, workflowId) to encapsulate component.workflow.getStatus parsing (producing the same {type,...} shape), and thin wrappers for cancelWorkflow, restartWorkflow, cleanupWorkflow, sendEventHelper and createEventHelper that perform the same ctx.runMutation calls used in BoundWorkflow (refer to methods start/status/cancel/restart/cleanup/sendEvent/createEvent for exact inputs/outputs); replace duplicated code in both BoundWorkflow and WorkflowManager with calls to these helpers so behavior remains identical.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@example/convex/userConfirmation.ts`:
- Around line 24-41: confirmationWorkflow currently indexes proposals with
approval.choice without validating it; before using proposals[approval.choice]
(after ctx.awaitEvent(approvalEvent) and proposals returned from
internal.userConfirmation.generateProposals) validate that approval.choice is a
safe integer and within 0 <= choice < proposals.length (no negatives, no
fractions, not out-of-range). If the check fails, reject early by returning a
clear error string or throwing a validation error instead of indexing; this
ensures invalid inputs from chooseProposal are handled before accessing
proposals.
In `@src/client/index.ts`:
- Around line 303-321: BoundWorkflow.sendEvent is not forwarding the optional
workpoolOptions to the event.send mutation like WorkflowManager.sendEvent does;
update the sendEvent implementation (the async sendEvent in BoundWorkflow) to
include workpoolOptions: config?.workpoolOptions (or the equivalent workflow
config variable) in the ctx.runMutation call so that component.event.send
receives the same workpoolOptions as WorkflowManager.sendEvent.
---
Outside diff comments:
In `@example/convex/example.ts`:
- Around line 9-21: The workflow-level default for retries was dropped; update
the defineWorkflow call for myWorkflow (the
defineWorkflow(...).bind(internal.example.exampleWorkflow) block) to include
workpoolOptions with retryActionsByDefault set to the previous default so
registered.workpoolOptions retains and merges that flag (e.g., add
workpoolOptions: { retryActionsByDefault: true } inside the defineWorkflow
options) ensuring getGeocoding and getWeather inherit the workflow-level retry
default.
---
Nitpick comments:
In `@src/client/index.ts`:
- Around line 235-327: BoundWorkflow methods (start, status, cancel, restart,
cleanup, sendEvent, createEvent) duplicate logic from WorkflowManager; factor
out shared helpers and call them from both surfaces. Implement small pure
helpers such as createWorkflowStart(component, ref, args, options) that
encapsulates createFunctionHandle, safeFunctionName and the ctx.runMutation call
to component.workflow.create (returning workflowId), getWorkflowStatus(ctx,
component, workflowId) to encapsulate component.workflow.getStatus parsing
(producing the same {type,...} shape), and thin wrappers for cancelWorkflow,
restartWorkflow, cleanupWorkflow, sendEventHelper and createEventHelper that
perform the same ctx.runMutation calls used in BoundWorkflow (refer to methods
start/status/cancel/restart/cleanup/sendEvent/createEvent for exact
inputs/outputs); replace duplicated code in both BoundWorkflow and
WorkflowManager with calls to these helpers so behavior remains identical.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 86dc4da8-a16e-40dd-9bbe-2e41af0318ed
📒 Files selected for processing (9)
example/convex/admin.tsexample/convex/catchError.tsexample/convex/example.tsexample/convex/nestedWorkflow.tsexample/convex/passingSignals.tsexample/convex/transcription.tsexample/convex/userConfirmation.tssrc/client/index.tssrc/client/workflowMutation.ts
| export const confirmationWorkflow = confirmationDef.handler( | ||
| async (ctx, args): Promise<string> => { | ||
| console.log("Starting confirmation workflow"); | ||
| const proposals = await step.runAction( | ||
| const proposals = await ctx.runAction( | ||
| internal.userConfirmation.generateProposals, | ||
| { prompt: args.prompt }, | ||
| { retry: true }, | ||
| ); | ||
| console.log("Proposals generated", proposals); | ||
| const approval = await step.awaitEvent(approvalEvent); | ||
| const approval = await ctx.awaitEvent(approvalEvent); | ||
| if (!approval.approved) { | ||
| return "rejected: " + approval.reason; | ||
| } | ||
| const choice = proposals[approval.choice]; | ||
| console.log("Choice selected", choice); | ||
| return choice; | ||
| }, | ||
| }); | ||
| ); |
There was a problem hiding this comment.
Reject invalid proposal indices before indexing proposals.
approval.choice is fed from chooseProposal, which currently accepts any v.number(). Negative, fractional, or out-of-range values make proposals[approval.choice] resolve to undefined, so this workflow fails later during return-value validation instead of rejecting the bad input cleanly.
Suggested fix
export const confirmationWorkflow = confirmationDef.handler(
async (ctx, args): Promise<string> => {
console.log("Starting confirmation workflow");
const proposals = await ctx.runAction(
internal.userConfirmation.generateProposals,
{ prompt: args.prompt },
{ retry: true },
);
console.log("Proposals generated", proposals);
const approval = await ctx.awaitEvent(approvalEvent);
if (!approval.approved) {
return "rejected: " + approval.reason;
}
+ if (
+ !Number.isInteger(approval.choice) ||
+ approval.choice < 0 ||
+ approval.choice >= proposals.length
+ ) {
+ throw new Error(`Invalid proposal index: ${approval.choice}`);
+ }
const choice = proposals[approval.choice];
console.log("Choice selected", choice);
return choice;
},
);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export const confirmationWorkflow = confirmationDef.handler( | |
| async (ctx, args): Promise<string> => { | |
| console.log("Starting confirmation workflow"); | |
| const proposals = await step.runAction( | |
| const proposals = await ctx.runAction( | |
| internal.userConfirmation.generateProposals, | |
| { prompt: args.prompt }, | |
| { retry: true }, | |
| ); | |
| console.log("Proposals generated", proposals); | |
| const approval = await step.awaitEvent(approvalEvent); | |
| const approval = await ctx.awaitEvent(approvalEvent); | |
| if (!approval.approved) { | |
| return "rejected: " + approval.reason; | |
| } | |
| const choice = proposals[approval.choice]; | |
| console.log("Choice selected", choice); | |
| return choice; | |
| }, | |
| }); | |
| ); | |
| export const confirmationWorkflow = confirmationDef.handler( | |
| async (ctx, args): Promise<string> => { | |
| console.log("Starting confirmation workflow"); | |
| const proposals = await ctx.runAction( | |
| internal.userConfirmation.generateProposals, | |
| { prompt: args.prompt }, | |
| { retry: true }, | |
| ); | |
| console.log("Proposals generated", proposals); | |
| const approval = await ctx.awaitEvent(approvalEvent); | |
| if (!approval.approved) { | |
| return "rejected: " + approval.reason; | |
| } | |
| if ( | |
| !Number.isInteger(approval.choice) || | |
| approval.choice < 0 || | |
| approval.choice >= proposals.length | |
| ) { | |
| throw new Error(`Invalid proposal index: ${approval.choice}`); | |
| } | |
| const choice = proposals[approval.choice]; | |
| console.log("Choice selected", choice); | |
| return choice; | |
| }, | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@example/convex/userConfirmation.ts` around lines 24 - 41,
confirmationWorkflow currently indexes proposals with approval.choice without
validating it; before using proposals[approval.choice] (after
ctx.awaitEvent(approvalEvent) and proposals returned from
internal.userConfirmation.generateProposals) validate that approval.choice is a
safe integer and within 0 <= choice < proposals.length (no negatives, no
fractions, not out-of-range). If the check fails, reject early by returning a
clear error string or throwing a validation error instead of indexing; this
ensures invalid inputs from chooseProposal are handled before accessing
proposals.
| async sendEvent(ctx, args) { | ||
| const result: RunResult = | ||
| "error" in args | ||
| ? { kind: "failed", error: args.error } | ||
| : { | ||
| kind: "success" as const, | ||
| returnValue: args.validator | ||
| ? parse(args.validator, args.value) | ||
| : "value" in args | ||
| ? args.value | ||
| : null, | ||
| }; | ||
| return (await ctx.runMutation(component.event.send, { | ||
| eventId: args.id, | ||
| result, | ||
| name: args.name, | ||
| workflowId: args.workflowId, | ||
| })) as any; | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check the event.send mutation signature for workpoolOptions parameter
ast-grep --pattern $'send = $_Mutation({
$$$
args: {
$$$
},
$$$
})'
# Also search for workpoolOptions usage in event handling
rg -n 'workpoolOptions' --type=ts -C3Repository: get-convex/workflow
Length of output: 11978
Missing workpoolOptions in BoundWorkflow.sendEvent compared to WorkflowManager.sendEvent.
The event.send mutation accepts an optional workpoolOptions parameter (defined in src/component/event.ts line 109 and used at line 172). However, the BoundWorkflow.sendEvent implementation at lines 303-321 does not pass this parameter to the mutation call, while WorkflowManager.sendEvent does (line 687). This creates inconsistent behavior between the two APIs.
If defineWorkflow config supports workpoolOptions, it should be forwarded to the mutation call in sendEvent:
Suggested fix
async sendEvent(ctx, args) {
const result: RunResult =
"error" in args
? { kind: "failed", error: args.error }
: {
kind: "success" as const,
returnValue: args.validator
? parse(args.validator, args.value)
: "value" in args
? args.value
: null,
};
return (await ctx.runMutation(component.event.send, {
eventId: args.id,
result,
name: args.name,
workflowId: args.workflowId,
workpoolOptions: config?.workpoolOptions,
})) as any;
},
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/client/index.ts` around lines 303 - 321, BoundWorkflow.sendEvent is not
forwarding the optional workpoolOptions to the event.send mutation like
WorkflowManager.sendEvent does; update the sendEvent implementation (the async
sendEvent in BoundWorkflow) to include workpoolOptions: config?.workpoolOptions
(or the equivalent workflow config variable) in the ctx.runMutation call so that
component.event.send receives the same workpoolOptions as
WorkflowManager.sendEvent.
cc5c138 to
77c50c8
Compare
commit: |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/client/index.ts`:
- Around line 211-213: The defineWorkflow() fluent API currently types its
config as only { args, returns } so per-workflow workpoolOptions get dropped;
update the config parameter type for defineWorkflow (the function that takes
component: WorkflowComponent and config) to include workpoolOptions (matching
WorkflowDefinition and workflowMutation), so callers can pass workpoolOptions
again, and ensure makeHandler()/workflowMutation() still receives the spread
config (no runtime change needed). Reference: modify the config type used in
defineWorkflow, the WorkflowComponent/WorkflowDefinition typings, and confirm
workflowMutation and makeHandler continue to accept and pass through
workpoolOptions.
In `@src/client/workflowMutation.ts`:
- Around line 64-74: The guard that checks for a bound-workflow fallback uses
the `in` operator on `args` which can throw for non-object values; update the
condition in the `if` (the branch containing `validate(workflowArgs, args)`,
`boundFn`, and the block that calls `makeFunctionReference`, `safeFunctionName`,
`createFunctionHandle`, and `ctx.runMutation` for `component.workflow.create`)
to first ensure `args` is a non-null object (e.g. `typeof args === "object" &&
args !== null`) before testing `"args" in args`, so malformed inputs (null,
numbers, strings) won't cause a TypeError and the code can surface
`INVALID_WORKFLOW_MESSAGE` as intended.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d55a13b2-a77e-4391-b641-6a9ba1958709
📒 Files selected for processing (9)
example/convex/admin.tsexample/convex/catchError.tsexample/convex/example.tsexample/convex/nestedWorkflow.tsexample/convex/passingSignals.tsexample/convex/transcription.tsexample/convex/userConfirmation.tssrc/client/index.tssrc/client/workflowMutation.ts
✅ Files skipped from review due to trivial changes (1)
- example/convex/admin.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- example/convex/catchError.ts
- example/convex/example.ts
| if (!validate(workflowArgs, args) && boundFn && "args" in args) { | ||
| // Bound workflow called directly with { args: ... } | ||
| const fn = makeFunctionReference(boundFn); | ||
| const workflowId = await ctx.runMutation(component.workflow.create, { | ||
| workflowName: safeFunctionName(fn), | ||
| workflowHandle: await createFunctionHandle(fn), | ||
| workflowArgs: args.args, | ||
| maxParallelism: workpoolOptions.maxParallelism, | ||
| }); | ||
| return workflowId; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In JavaScript/TypeScript, what happens when the inoperator is used withnull, undefined, or a primitive on the right-hand side? Please cite MDN or the ECMAScript specification.
💡 Result:
When the in operator is used with null, undefined, or any primitive value (such as a number, string, boolean, symbol, or bigint) on the right-hand side, it throws a TypeError exception. This behavior is specified in the ECMAScript specification in the evaluation semantics for the production RelationalExpression in ShiftExpression (see ECMAScript 2026 #sec-relational-operators). The steps are: 1. Let lRef be ? Evaluation of RelationalExpression. 2. Let lVal be ? GetValue(lRef). 3. Let rRef be ? Evaluation of ShiftExpression. 4. Let rVal be ? GetValue(rRef). 5. If Type(rVal) is not Object, throw a TypeError exception. 6. Return ? HasProperty(rVal, ? ToPropertyKey(lVal)). Since null, undefined, and primitives have types other than Object, step 5 throws TypeError. No object coercion (like ToObject) is performed on the right-hand side; it must already be an Object. MDN confirms: "TypeError: Thrown if object is not an object (i.e., a primitive)." and provides examples like "bar" in null throwing TypeError.
Citations:
- 1: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/in
- 2: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Errors/in_operator_no_object
- 3: https://tc39.es/ecma262/multipage/ecmascript-language-expressions.html
Guard the bound-workflow fallback before using in.
Line 64 evaluates "args" in args without checking the type of args. If a malformed input such as null, a number, or a string reaches this condition, the in operator will throw a TypeError instead of allowing the code to surface the intended INVALID_WORKFLOW_MESSAGE.
Suggested fix
- if (!validate(workflowArgs, args) && boundFn && "args" in args) {
+ if (
+ !validate(workflowArgs, args) &&
+ boundFn &&
+ typeof args === "object" &&
+ args !== null &&
+ "args" in args
+ ) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (!validate(workflowArgs, args) && boundFn && "args" in args) { | |
| // Bound workflow called directly with { args: ... } | |
| const fn = makeFunctionReference(boundFn); | |
| const workflowId = await ctx.runMutation(component.workflow.create, { | |
| workflowName: safeFunctionName(fn), | |
| workflowHandle: await createFunctionHandle(fn), | |
| workflowArgs: args.args, | |
| maxParallelism: workpoolOptions.maxParallelism, | |
| }); | |
| return workflowId; | |
| } | |
| if ( | |
| !validate(workflowArgs, args) && | |
| boundFn && | |
| typeof args === "object" && | |
| args !== null && | |
| "args" in args | |
| ) { | |
| // Bound workflow called directly with { args: ... } | |
| const fn = makeFunctionReference(boundFn); | |
| const workflowId = await ctx.runMutation(component.workflow.create, { | |
| workflowName: safeFunctionName(fn), | |
| workflowHandle: await createFunctionHandle(fn), | |
| workflowArgs: args.args, | |
| maxParallelism: workpoolOptions.maxParallelism, | |
| }); | |
| return workflowId; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/client/workflowMutation.ts` around lines 64 - 74, The guard that checks
for a bound-workflow fallback uses the `in` operator on `args` which can throw
for non-object values; update the condition in the `if` (the branch containing
`validate(workflowArgs, args)`, `boundFn`, and the block that calls
`makeFunctionReference`, `safeFunctionName`, `createFunctionHandle`, and
`ctx.runMutation` for `component.workflow.create`) to first ensure `args` is a
non-null object (e.g. `typeof args === "object" && args !== null`) before
testing `"args" in args`, so malformed inputs (null, numbers, strings) won't
cause a TypeError and the code can surface `INVALID_WORKFLOW_MESSAGE` as
intended.
There was a problem hiding this comment.
args is always an object in Convex
There was a problem hiding this comment.
@ianmacartney, that makes sense — since Convex guarantees that mutation args are always a non-null object at the framework level, the typeof args === "object" && args !== null guard is indeed redundant here. Thanks for the clarification!
✏️ Learnings added
Learnt from: ianmacartney
Repo: get-convex/workflow PR: 230
File: src/client/workflowMutation.ts:65-75
Timestamp: 2026-04-10T08:07:19.691Z
Learning: In Convex, the `args` parameter passed to mutation (and query/action) handlers is always a non-null object — the framework enforces this at the runtime level. Guards like `typeof args === "object" && args !== null` before using the `in` operator on `args` are therefore unnecessary in Convex handler code.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
ianmacartney
force-pushed
the
ian/define-workflow
branch
from
81d300e to
5284b4d
Compare
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
example/convex/e2e.ts (1)
31-53:⚠️ Potential issue | 🟠 MajorDon't bypass
startWorkflowfor the weather example.
example/convex/example.ts'sstartWorkflowalso inserts theflowsrow and wiresonComplete: internal.example.flowCompleted. StartingmyWorkflowdirectly here skips that setup, sointernal.example.updateFlowwill logFlow not foundand the result never gets patched back intoflows.💡 Suggested fix
export const startAll = mutation({ args: {}, returns: v.object({ weather: v.string(), @@ }), handler: async (ctx) => { - const weather = await myWorkflow.start( - ctx, - { location: "San Jose" }, - { startAsync: true }, - ); + const location = "San Jose"; + const weather = await myWorkflow.start( + ctx, + { location }, + { + onComplete: internal.example.flowCompleted, + context: { location }, + startAsync: true, + }, + ); + await ctx.db.insert("flows", { + workflowId: weather, + in: location, + out: null, + }); const catchError = await catchErrorWorkflow.start(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@example/convex/e2e.ts` around lines 31 - 53, The weather example is incorrectly starting the workflow directly with myWorkflow.start which bypasses the startWorkflow helper that inserts the flows row and wires onComplete -> internal.example.flowCompleted; update internal.example.updateFlow expects that row. Replace the myWorkflow.start call with the project's startWorkflow invocation (the same helper used in example/convex/example.ts) for the weather run so the flows row is created and onComplete is registered, ensuring internal.example.updateFlow can find and patch the flow result.example/convex/catchError.ts (1)
19-36:⚠️ Potential issue | 🟡 MinorReject non-integer retry counts.
manualRetriesis used as a loop bound, butv.number()still accepts values like-1or0.5. That makes this handler do 0 or 2 attempts for inputs that do not make sense as a retry count.💡 Suggested fix
export const catchError = catchErrorWorkflow.handler( async (step, args): Promise<number> => { + if (!Number.isInteger(args.manualRetries) || args.manualRetries < 0) { + throw new Error("manualRetries must be a non-negative integer"); + } let i; for (i = 0; i < args.manualRetries + 1; i++) { try {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@example/convex/catchError.ts` around lines 19 - 36, The handler uses args.manualRetries as a loop bound but doesn't validate it; add an early validation in the async handler (the function passed to catchErrorWorkflow.handler) to ensure Number.isInteger(args.manualRetries) and args.manualRetries >= 0, and throw a clear Error (e.g., "manualRetries must be a non-negative integer") if the check fails so the loop never runs with invalid values.
♻️ Duplicate comments (1)
example/convex/userConfirmation.ts (1)
24-41:⚠️ Potential issue | 🟡 MinorValidate
approval.choicebefore indexingproposals.Negative, fractional, or out-of-range values still make
proposals[approval.choice]becomeundefined, so the workflow fails later on thev.string()return validation instead of rejecting the bad event cleanly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@example/convex/userConfirmation.ts` around lines 24 - 41, The handler registered with confirmationWorkflow.handler should validate approval.choice before indexing proposals: after awaiting ctx.awaitEvent(approvalEvent) check that Number.isInteger(approval.choice) and approval.choice >= 0 and approval.choice < proposals.length; if the check fails return a rejection string (e.g., "rejected: invalid choice") instead of indexing proposals[approval.choice]. Update the confirmation function (the async handler using internal.userConfirmation.generateProposals, ctx.awaitEvent, and returning a string) to perform this bounds/type check so negative, fractional, or out-of-range values are handled cleanly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@example/convex/example.ts`:
- Around line 23-37: The two network-backed actions inside the example workflow
need explicit retry options: update the first step.runAction call to
internal.example.getGeocoding (which currently only has { runAfter: 100 }) to
include retry (e.g., { runAfter: 100, retry: true } or a retry config object),
and add a retry option to the getWeather call
(step.runAction(internal.example.getWeather, { latitude, longitude }) → include
retry: true or a retry config). Ensure you modify the example handler function
(export const example = myWorkflow.handler(...)) so both
internal.example.getGeocoding and internal.example.getWeather calls have retry
configured to handle transient HTTP failures.
In `@example/convex/transcription.ts`:
- Around line 18-22: The workflow defined as transcriptionDef using
defineWorkflow(...).bind(internal.transcription.transcriptionWorkflow) loses the
ability to pass workpoolOptions (e.g., maxParallelism), so switch to creating a
WorkflowDefinition/WorkflowManager that allows specifying workpoolOptions or
revert to the legacy API: replace the defineWorkflow(...).bind(...) usage with a
WorkflowDefinition (or WorkflowManager.register) that references
internal.transcription.transcriptionWorkflow and include a workpoolOptions
object containing maxParallelism (and any defaultWorkpoolOptions) so
computeTranscription/computeEmbedding steps are rate-limited.
---
Outside diff comments:
In `@example/convex/catchError.ts`:
- Around line 19-36: The handler uses args.manualRetries as a loop bound but
doesn't validate it; add an early validation in the async handler (the function
passed to catchErrorWorkflow.handler) to ensure
Number.isInteger(args.manualRetries) and args.manualRetries >= 0, and throw a
clear Error (e.g., "manualRetries must be a non-negative integer") if the check
fails so the loop never runs with invalid values.
In `@example/convex/e2e.ts`:
- Around line 31-53: The weather example is incorrectly starting the workflow
directly with myWorkflow.start which bypasses the startWorkflow helper that
inserts the flows row and wires onComplete -> internal.example.flowCompleted;
update internal.example.updateFlow expects that row. Replace the
myWorkflow.start call with the project's startWorkflow invocation (the same
helper used in example/convex/example.ts) for the weather run so the flows row
is created and onComplete is registered, ensuring internal.example.updateFlow
can find and patch the flow result.
---
Duplicate comments:
In `@example/convex/userConfirmation.ts`:
- Around line 24-41: The handler registered with confirmationWorkflow.handler
should validate approval.choice before indexing proposals: after awaiting
ctx.awaitEvent(approvalEvent) check that Number.isInteger(approval.choice) and
approval.choice >= 0 and approval.choice < proposals.length; if the check fails
return a rejection string (e.g., "rejected: invalid choice") instead of indexing
proposals[approval.choice]. Update the confirmation function (the async handler
using internal.userConfirmation.generateProposals, ctx.awaitEvent, and returning
a string) to perform this bounds/type check so negative, fractional, or
out-of-range values are handled cleanly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e6d052a3-60d2-4331-bcbb-d425ae299f10
📒 Files selected for processing (10)
example/convex/catchError.tsexample/convex/e2e.tsexample/convex/example.test.tsexample/convex/example.tsexample/convex/inlineTest.test.tsexample/convex/inlineTest.tsexample/convex/nestedWorkflow.tsexample/convex/passingSignals.tsexample/convex/transcription.tsexample/convex/userConfirmation.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- example/convex/nestedWorkflow.ts
| export const example = myWorkflow.handler(async (step, args) => { | ||
| console.time("overall"); | ||
| console.time("geocoding"); | ||
| // Run in parallel! | ||
| const [{ latitude, longitude, name }, weather2] = await Promise.all([ | ||
| step.runAction(internal.example.getGeocoding, args, { runAfter: 100 }), | ||
| step.runAction(internal.example.getGeocoding, args, { retry: true }), | ||
| ]); | ||
| console.log("Is geocoding is consistent?", latitude === weather2.latitude); | ||
| console.timeLog("geocoding", name); | ||
| console.time("weather"); | ||
| const weather = await step.runAction(internal.example.getWeather, { | ||
| latitude, | ||
| longitude, | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd -t f example.tsRepository: get-convex/workflow
Length of output: 88
🏁 Script executed:
cat -n example/convex/example.tsRepository: get-convex/workflow
Length of output: 7034
🏁 Script executed:
rg -i "workflow.*retry\|retry.*workflow" --type ts --type js -A 3 -B 3Repository: get-convex/workflow
Length of output: 45
🏁 Script executed:
rg "defineWorkflow" -A 10 -B 2 --type ts --type js | head -100Repository: get-convex/workflow
Length of output: 5822
🏁 Script executed:
rg "runAction" -A 5 -B 2 --type ts --type js example/convex/ | head -150Repository: get-convex/workflow
Length of output: 3503
🏁 Script executed:
rg "runAction" -B 5 -A 10 "src/client" --type ts | head -200Repository: get-convex/workflow
Length of output: 11786
🏁 Script executed:
rg "RetryOption\|interface.*Retry\|type.*Retry" -A 5 -B 2 "src/" --type ts | head -150Repository: get-convex/workflow
Length of output: 45
🏁 Script executed:
rg "retry" -i -A 5 -B 2 "src/client" --type ts | head -200Repository: get-convex/workflow
Length of output: 7135
🏁 Script executed:
rg "type RetryOption\|interface RetryOption" -A 10 --type tsRepository: get-convex/workflow
Length of output: 45
🏁 Script executed:
cat -n src/client/workflowContext.ts | sed -n '1,30p'Repository: get-convex/workflow
Length of output: 1370
🏁 Script executed:
find . -name "workpool" -o -name "*workpool*" -type dRepository: get-convex/workflow
Length of output: 45
🏁 Script executed:
rg "RetryOption\|retry.*default\|default.*retry" -i -A 3 -B 3 --type ts --type md | head -150Repository: get-convex/workflow
Length of output: 45
🏁 Script executed:
fd -t f "README\|readme\|CHANGELOG\|changelog\|doc" | head -20Repository: get-convex/workflow
Length of output: 45
🏁 Script executed:
cat -n package.json | head -50Repository: get-convex/workflow
Length of output: 2562
🏁 Script executed:
rg "retry.*undefined\|undefined.*retry" -B 5 -A 5 --type ts src/client/Repository: get-convex/workflow
Length of output: 45
🏁 Script executed:
cat -n src/client/step.ts | head -200Repository: get-convex/workflow
Length of output: 7964
Add explicit retries to the network-backed steps.
The first getGeocoding call and the getWeather call both depend on external HTTP requests, but lack retry configuration. Only one getGeocoding call in this block has { retry: true }. A transient timeout or 5xx error will fail the workflow immediately. Add explicit retry options to both calls.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@example/convex/example.ts` around lines 23 - 37, The two network-backed
actions inside the example workflow need explicit retry options: update the
first step.runAction call to internal.example.getGeocoding (which currently only
has { runAfter: 100 }) to include retry (e.g., { runAfter: 100, retry: true } or
a retry config object), and add a retry option to the getWeather call
(step.runAction(internal.example.getWeather, { latitude, longitude }) → include
retry: true or a retry config). Ensure you modify the example handler function
(export const example = myWorkflow.handler(...)) so both
internal.example.getGeocoding and internal.example.getWeather calls have retry
configured to handle transient HTTP failures.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
example/convex/example.ts (1)
28-37:⚠️ Potential issue | 🟠 MajorAdd retry config to all external HTTP steps.
Line 28 and Line 34 still call network-backed actions without retry options, so transient failures can fail the workflow immediately. This was flagged previously and still appears unresolved.
Proposed fix
const [{ latitude, longitude, name }, weather2] = await Promise.all([ - step.runAction(internal.example.getGeocoding, args, { runAfter: 100 }), + step.runAction(internal.example.getGeocoding, args, { + runAfter: 100, + retry: true, + }), step.runAction(internal.example.getGeocoding, args, { retry: true }), ]); @@ - const weather = await step.runAction(internal.example.getWeather, { - latitude, - longitude, - }); + const weather = await step.runAction( + internal.example.getWeather, + { latitude, longitude }, + { retry: true }, + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@example/convex/example.ts` around lines 28 - 37, The calls to external network-backed actions lack retry configuration: add a retry option to all step.runAction calls that invoke network actions (specifically the two invocations of internal.example.getGeocoding and the call to internal.example.getWeather) so transient HTTP failures won't abort the workflow; update each invocation of step.runAction(internal.example.getGeocoding, ...) and step.runAction(internal.example.getWeather, ...) to include a retry policy (e.g., retry: true or a retry object with attempts/backoff) consistent with other steps in this file.
🧹 Nitpick comments (2)
example/convex/transcription.ts (1)
36-52: Consider consistent handler parameter naming across workflow files.This file uses
stepas the handler parameter name, while other files likenestedWorkflow.tsanduserConfirmation.tsusectx. Both work identically, but consistent naming improves codebase readability.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@example/convex/transcription.ts` around lines 36 - 52, The handler parameter in transcriptionWorkflow uses the name "step" which is inconsistent with other workflows that use "ctx"; update transcriptionWorkflow's transcriptionDef.handler parameter from "step" to "ctx" and replace all uses of step.runAction(...) with ctx.runAction(...), keeping the same action calls (internal.transcription.computeTranscription and internal.transcription.computeEmbedding) and options (storageId, retry: false) so behavior is unchanged.example/convex/example.test.ts (1)
22-28: Minor inconsistency:t.mutation()vst.run()for starting workflows.This test uses
t.mutation()to start the workflow, while other tests (e.g., nested workflow at line 60) uset.run(). Both work, but consistent usage improves readability.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@example/convex/example.test.ts` around lines 22 - 28, Replace the inconsistent call to start the workflow: instead of using t.mutation(...) to invoke catchErrorWorkflow.start, call t.run(...) so it matches the other tests (e.g., nested workflow) and improves readability; update the invocation of catchErrorWorkflow.start(ctx, { manualRetries: 3 }) to be passed into t.run in place of t.mutation while keeping the subsequent calls (t.finishAllScheduledFunctions and catchErrorWorkflow.status) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@example/convex/test/oldSyntax.ts`:
- Line 7: The import line currently brings in WorkflowManager and vWorkflowId
but vWorkflowId is unused; update the import statement that references
WorkflowManager and vWorkflowId to remove vWorkflowId so only WorkflowManager is
imported (i.e., change the import from "@convex-dev/workflow" to import just
WorkflowManager) to eliminate the unused symbol.
---
Duplicate comments:
In `@example/convex/example.ts`:
- Around line 28-37: The calls to external network-backed actions lack retry
configuration: add a retry option to all step.runAction calls that invoke
network actions (specifically the two invocations of
internal.example.getGeocoding and the call to internal.example.getWeather) so
transient HTTP failures won't abort the workflow; update each invocation of
step.runAction(internal.example.getGeocoding, ...) and
step.runAction(internal.example.getWeather, ...) to include a retry policy
(e.g., retry: true or a retry object with attempts/backoff) consistent with
other steps in this file.
---
Nitpick comments:
In `@example/convex/example.test.ts`:
- Around line 22-28: Replace the inconsistent call to start the workflow:
instead of using t.mutation(...) to invoke catchErrorWorkflow.start, call
t.run(...) so it matches the other tests (e.g., nested workflow) and improves
readability; update the invocation of catchErrorWorkflow.start(ctx, {
manualRetries: 3 }) to be passed into t.run in place of t.mutation while keeping
the subsequent calls (t.finishAllScheduledFunctions and
catchErrorWorkflow.status) unchanged.
In `@example/convex/transcription.ts`:
- Around line 36-52: The handler parameter in transcriptionWorkflow uses the
name "step" which is inconsistent with other workflows that use "ctx"; update
transcriptionWorkflow's transcriptionDef.handler parameter from "step" to "ctx"
and replace all uses of step.runAction(...) with ctx.runAction(...), keeping the
same action calls (internal.transcription.computeTranscription and
internal.transcription.computeEmbedding) and options (storageId, retry: false)
so behavior is unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6e32ec91-7ddb-4e01-940c-6404b0c93ab3
⛔ Files ignored due to path filters (1)
example/convex/_generated/api.d.tsis excluded by!**/_generated/**
📒 Files selected for processing (11)
example/convex/catchError.tsexample/convex/e2e.tsexample/convex/example.test.tsexample/convex/example.tsexample/convex/inlineTest.test.tsexample/convex/inlineTest.tsexample/convex/nestedWorkflow.tsexample/convex/passingSignals.tsexample/convex/test/oldSyntax.tsexample/convex/transcription.tsexample/convex/userConfirmation.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- example/convex/passingSignals.ts
ianmacartney
force-pushed
the
ian/define-workflow
branch
from
6346951 to
c4950d2
Compare
The workflow API now supports two patterns:
1. Direct: `workflow.define({...}).handler(fn)` - returns a
registered mutation to export
2. With reference: `workflow.define({...}).withHandlerRef(ref)` -
returns a Workflow object with `.start()` method
Removed workflow management methods (status, cancel, restart, list,
etc.) from the Workflow interface in favor of standalone functions.
Updated documentation and examples to reflect the new API.
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
example/convex/transcription.ts (1)
17-23:⚠️ Potential issue | 🟠 MajorCarry forward the workflow’s
workpoolOptions.This fluent rewrite drops the workflow-level concurrency settings, which changes runtime behavior for the OpenAI-backed steps below. If this example previously capped parallelism to avoid rate-limit/cost spikes, that protection needs to move into
.define({ ..., workpoolOptions })rather than disappear in the migration.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@example/convex/transcription.ts` around lines 17 - 23, The workflow rewrite removed the workflow-level concurrency settings; update the transcriptionWorkflow declaration (the call to workflow.define that currently sets args { storageId: v.id("_storage") } and then .handler) to include the original workpoolOptions so the OpenAI-backed steps keep their prior concurrency cap—add a workpoolOptions property to the define call (using the same maxConcurrent / concurrency values that were used before) so the workflow-level parallelism is preserved.
🧹 Nitpick comments (1)
example/convex/test/oldSyntax.ts (1)
85-90: Consider tightening the event-wait detection.The check
"kind" in r && r.kind === "event"safely detects an awaiting event step, but it relies on the internal structure ofStep. If the schema changes, this test could silently pass the loop without detecting the event wait. A comment clarifying the expected structure would improve maintainability.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@example/convex/test/oldSyntax.ts` around lines 85 - 90, The loop’s check for an event wait is brittle because it inspects a presumed internal shape ("kind" in r && r.kind === "event"); update the detection to a strict, explicit type guard (e.g., implement and call an isEventStep(r) helper that narrows the Step type or checks stable discriminant fields) and replace the inline condition in the loop that calls workflow.status(ctx, workflowId) and examines s.running with that helper; also add a short comment above the helper describing the expected Step discriminants so future schema changes are obvious.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@example/convex/admin.ts`:
- Around line 11-13: The current return calls the low-level query
components.workflow.workflow.getStatus which returns the raw { workflow,
inProgress } payload; change it to use the public helper that returns the
normalized WorkflowStatus (so callers get status.type and status.result).
Replace the ctx.runQuery target from components.workflow.workflow.getStatus to
the helper components.workflow.getWorkflowStatus and pass the same { workflowId:
args.workflowId } so the function returns the expected normalized status shape.
In `@README.md`:
- Around line 468-469: The README sentence contains a typo: replace
"immmediately" with "immediately" in the sentence that mentions cancel() and
step.runAction(); update the text near the phrase "You can cancel a workflow
with `cancel()`, halting the workflow's execution immmediately." so it reads
"immediately" to fix the spelling error.
- Around line 451-458: The example in runWorkflowAndPoll is using an
out-of-scope variable and the wrong discriminant for WorkflowStatus: call
getStatus with the provided args (e.g., getStatus(ctx, components.workflow,
args.workflowId) or use vWorkflowId consistently) and check status.type rather
than status.kind (e.g., status.type === "inProgress" ? status.running : []).
Update the two lines referencing workflowId and status.kind to use
args.workflowId (or vWorkflowId) and status.type so the snippet compiles and
matches the WorkflowStatus shape.
In `@src/client/index.ts`:
- Around line 182-198: The start method in withHandlerRef().start (using
createFunctionHandle(ref), safeFunctionName(ref), and creating onComplete
handle) fails to forward maxParallelism, so change the args passed to
ctx.runMutation(component.workflow.create, {...}) to include maxParallelism:
options?.config?.workpoolOptions?.maxParallelism (or
options?.workpoolOptions?.maxParallelism if your options shape differs); ensure
the key is named maxParallelism in the payload and preserve existing fields
(workflowName, workflowHandle, workflowArgs, onComplete, startAsync) when adding
this new property.
In `@src/client/workflowMutation.ts`:
- Line 58: The RegisteredMutation return type is declared as void but the
handler actually returns a workflowId; update the generic return type so callers
see the correct value: change RegisteredMutation<"internal",
WorkflowArgs<ArgsValidator>, void> to RegisteredMutation<"internal",
WorkflowArgs<ArgsValidator>, WorkflowId> (or the appropriate WorkflowId type)
and remove/avoid the downstream as any cast that masks the mismatch; update any
related exported types or wrapper functions that assume void so the
bound-workflow and fn-based paths correctly propagate the WorkflowId return.
---
Duplicate comments:
In `@example/convex/transcription.ts`:
- Around line 17-23: The workflow rewrite removed the workflow-level concurrency
settings; update the transcriptionWorkflow declaration (the call to
workflow.define that currently sets args { storageId: v.id("_storage") } and
then .handler) to include the original workpoolOptions so the OpenAI-backed
steps keep their prior concurrency cap—add a workpoolOptions property to the
define call (using the same maxConcurrent / concurrency values that were used
before) so the workflow-level parallelism is preserved.
---
Nitpick comments:
In `@example/convex/test/oldSyntax.ts`:
- Around line 85-90: The loop’s check for an event wait is brittle because it
inspects a presumed internal shape ("kind" in r && r.kind === "event"); update
the detection to a strict, explicit type guard (e.g., implement and call an
isEventStep(r) helper that narrows the Step type or checks stable discriminant
fields) and replace the inline condition in the loop that calls
workflow.status(ctx, workflowId) and examines s.running with that helper; also
add a short comment above the helper describing the expected Step discriminants
so future schema changes are obvious.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8d14e96c-880b-4a97-a6bb-d1611647daa4
⛔ Files ignored due to path filters (1)
example/convex/_generated/api.d.tsis excluded by!**/_generated/**
📒 Files selected for processing (15)
README.mdexample/convex/admin.tsexample/convex/catchError.tsexample/convex/e2e.tsexample/convex/example.test.tsexample/convex/example.tsexample/convex/inlineTest.test.tsexample/convex/inlineTest.tsexample/convex/nestedWorkflow.tsexample/convex/passingSignals.tsexample/convex/test/oldSyntax.tsexample/convex/transcription.tsexample/convex/userConfirmation.tssrc/client/index.tssrc/client/workflowMutation.ts
✅ Files skipped from review due to trivial changes (1)
- example/convex/inlineTest.test.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- example/convex/example.ts
- example/convex/catchError.ts
- example/convex/userConfirmation.ts
| async start(ctx, args, options?) { | ||
| const handle = await createFunctionHandle(ref); | ||
| const onComplete = options?.onComplete | ||
| ? { | ||
| fnHandle: await createFunctionHandle(options.onComplete), | ||
| context: options.context, | ||
| } | ||
| : undefined; | ||
| const workflowId = await ctx.runMutation(component.workflow.create, { | ||
| workflowName: safeFunctionName(ref), | ||
| workflowHandle: handle, | ||
| workflowArgs: args, | ||
| onComplete, | ||
| startAsync: options?.startAsync, | ||
| }); | ||
| return workflowId as unknown as WorkflowId; | ||
| }, |
There was a problem hiding this comment.
withHandlerRef().start() does not forward maxParallelism.
Unlike WorkflowManager.start (line 646), the start method here does not pass config.workpoolOptions?.maxParallelism to component.workflow.create. Users migrating from WorkflowManager to the new fluent API will lose their parallelism configuration.
Suggested fix
const workflowId = await ctx.runMutation(component.workflow.create, {
workflowName: safeFunctionName(ref),
workflowHandle: handle,
workflowArgs: args,
+ maxParallelism: config.workpoolOptions?.maxParallelism,
onComplete,
startAsync: options?.startAsync,
});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/client/index.ts` around lines 182 - 198, The start method in
withHandlerRef().start (using createFunctionHandle(ref), safeFunctionName(ref),
and creating onComplete handle) fails to forward maxParallelism, so change the
args passed to ctx.runMutation(component.workflow.create, {...}) to include
maxParallelism: options?.config?.workpoolOptions?.maxParallelism (or
options?.workpoolOptions?.maxParallelism if your options shape differs); ensure
the key is named maxParallelism in the payload and preserve existing fields
(workflowName, workflowHandle, workflowArgs, onComplete, startAsync) when adding
this new property.
| > { | ||
| defaultWorkpoolOptions?: WorkpoolOptions, | ||
| boundFn?: string, | ||
| ): RegisteredMutation<"internal", WorkflowArgs<ArgsValidator>, void> { |
There was a problem hiding this comment.
Return type declares void but handler returns workflowId.
The function signature at line 58 specifies RegisteredMutation<..., void>, yet lines 74 and 87 return workflowId. The as any cast at line 205 masks this at compile time, but callers using the bound-workflow or fn-based paths will receive a WorkflowId while the type promises void.
Consider updating the return type to reflect the actual behavior, or ensure the mutation wrapper properly handles the return value.
Also applies to: 74-74, 87-87
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/client/workflowMutation.ts` at line 58, The RegisteredMutation return
type is declared as void but the handler actually returns a workflowId; update
the generic return type so callers see the correct value: change
RegisteredMutation<"internal", WorkflowArgs<ArgsValidator>, void> to
RegisteredMutation<"internal", WorkflowArgs<ArgsValidator>, WorkflowId> (or the
appropriate WorkflowId type) and remove/avoid the downstream as any cast that
masks the mismatch; update any related exported types or wrapper functions that
assume void so the bound-workflow and fn-based paths correctly propagate the
WorkflowId return.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@example/convex/test/oldSyntax.ts`:
- Around line 86-98: The polling loop that checks workflow.status (the for loop
before calling workflow.sendEvent) can fall through after 100 tries and proceed
to send the "approval" event even if the workflow never reached the awaitEvent
wait state; change the loop so it fails fast: detect when the loop exits without
seeing a running entry with kind === "event" (e.g., set a boolean flag or check
the loop outcome) and throw an error (including workflowId and current status)
instead of calling workflow.sendEvent; reference the existing loop,
workflow.status(ctx, workflowId), and workflow.sendEvent(ctx, { workflowId,
name: "approval", ... }) to locate and modify the code.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2c6f443b-0a83-466b-ad6b-bbd9c97e2475
📒 Files selected for processing (3)
example/convex/test/oldSyntax.tsexample/e2e.mjspackage.json
| for (let i = 0; i < 100; i++) { | ||
| await new Promise((r) => setTimeout(r, 100)); | ||
| const s = await workflow.status(ctx, workflowId); | ||
| if (s.type !== "inProgress") throw new Error(`Unexpected: ${s.type}`); | ||
| if (s.running.some((r) => "kind" in r && r.kind === "event")) break; | ||
| } | ||
|
|
||
| // workflow.sendEvent | ||
| await workflow.sendEvent(ctx, { | ||
| workflowId, | ||
| name: "approval", | ||
| value: "approved!", | ||
| }); |
There was a problem hiding this comment.
Fail fast if the workflow never reaches the approval wait state.
The first polling loop falls through after 100 tries and still sends "approval". That hides the real regression when awaitEvent is never reached.
Proposed fix
- for (let i = 0; i < 100; i++) {
+ let waitingForApproval = false;
+ for (let i = 0; i < 100; i++) {
await new Promise((r) => setTimeout(r, 100));
const s = await workflow.status(ctx, workflowId);
if (s.type !== "inProgress") throw new Error(`Unexpected: ${s.type}`);
- if (s.running.some((r) => "kind" in r && r.kind === "event")) break;
+ if (s.running.some((r) => "kind" in r && r.kind === "event")) {
+ waitingForApproval = true;
+ break;
+ }
}
+ if (!waitingForApproval) {
+ throw new Error("Workflow never reached the approval event step");
+ }
// workflow.sendEvent
await workflow.sendEvent(ctx, {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@example/convex/test/oldSyntax.ts` around lines 86 - 98, The polling loop that
checks workflow.status (the for loop before calling workflow.sendEvent) can fall
through after 100 tries and proceed to send the "approval" event even if the
workflow never reached the awaitEvent wait state; change the loop so it fails
fast: detect when the loop exits without seeing a running entry with kind ===
"event" (e.g., set a boolean flag or check the loop outcome) and throw an error
(including workflowId and current status) instead of calling workflow.sendEvent;
reference the existing loop, workflow.status(ctx, workflowId), and
workflow.sendEvent(ctx, { workflowId, name: "approval", ... }) to locate and
modify the code.
1 participant
This change introduces a new fluent API for defining workflows using
defineWorkflow()that separates workflow definition from handler implementation. The new pattern avoids type cycles.Key Changes
defineWorkflow()function that returns anUnboundWorkflowwhich can be bound to a function reference.bind(ref)to create aBoundWorkflowwith management methods like.start(),.status(), and.cancel()directly on themWorkflowManager.define().handler()methodMigration Pattern
Before:
Now:
Also possible but not documented/ showcased yet:
The new API provides better ergonomics for workflow management while maintaining backward compatibility through the existing
WorkflowManagerclass.