Open
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 7 minutes and 0 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (9)
📝 WalkthroughWalkthroughAdds a typed inline step API (WorkflowCtx.run) with inline step journaling, threads a DataModel generic and optional internalMutation through workflow types and runtime, enforces a guard against nested step calls, updates step/journal/workflow mappings, expands tests, and adds an event-timeout example with related internal mutations. Changes
Sequence DiagramsequenceDiagram
participant WF as Workflow
participant Ctx as WorkflowCtx
participant Journal as Journal
participant Executor as StepExecutor
WF->>Ctx: ctx.run(handler, opts)
Ctx->>Ctx: inlineDepth += 1
Ctx->>Journal: enqueue StepRequest { kind: "inline", handler, args, name/deps }
Journal->>Executor: poll step
alt inline handler executes locally
Executor->>Executor: execute handler(GenericMutationCtx)
Executor->>Journal: mark step complete (persist as function result)
Journal->>Ctx: deliver result
end
Ctx->>Ctx: inlineDepth -= 1
Ctx-->>WF: resolve with handler result
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested Reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
commit: |
Member
Author
|
@cursor review |
|
Skipping Bugbot: Bugbot is disabled for this repository. Visit the Bugbot dashboard to update your settings. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
example/convex/example.ts (1)
53-63: Prefer journaling the captured values in this inline example.This handler closes over
step.workflowIdand the computed weather fields but journals{}. On replay that means the step can’t detect that those captured values changed, which is a rough pattern to teach in the example users will copy.Suggested change
- await step.run(async (ctx) => { - const flow = await ctx.db - .query("flows") - .withIndex("workflowId", (q) => q.eq("workflowId", step.workflowId)) - .first(); - if (flow) { - await ctx.db.patch(flow._id, { - out: { name, celsius, farenheit, windSpeed, windGust }, - }); - } - }); + await step.run( + async (ctx) => { + const flow = await ctx.db + .query("flows") + .withIndex("workflowId", (q) => q.eq("workflowId", step.workflowId)) + .first(); + if (flow) { + await ctx.db.patch(flow._id, { + out: { name, celsius, farenheit, windSpeed, windGust }, + }); + } + }, + { + deps: { + workflowId: step.workflowId, + name, + celsius, + farenheit, + windSpeed, + windGust, + }, + }, + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@example/convex/example.ts` around lines 53 - 63, The inline handler closes over step.workflowId and computed weather fields but writes an empty journaled object, so changes to those captured values won’t be detected on replay; fix it by capturing the needed values into locals before calling step.run (e.g., const capturedWorkflowId = step.workflowId; const capturedOut = { name, celsius, farenheit, windSpeed, windGust }) or by passing them into the step.run closure, and then include those captured values in the ctx.db.patch call (set out to the capturedOut and/or include capturedWorkflowId) so step.run, ctx.db.query("flows").withIndex(...), and ctx.db.patch persist the actual captured inputs for proper journaling and replay detection.src/client/step.ts (1)
218-255: Deeply nested ternary chain is complex but handles all cases exhaustively.The step construction logic correctly:
- Maps inline steps to
kind: "function"with sentinelhandle: "inline"(lines 219-225)- Uses an IIFE for exhaustive type checking at line 251-255, ensuring compile-time errors if new target kinds are added
The
handle: "inline"sentinel value is appropriate since inline handlers don't have a function reference to create a handle from.Consider extracting this into a helper function or using a switch statement for improved readability, though the current approach is functional and provides exhaustive checking.
♻️ Optional: Extract to switch statement for clarity
- const step = - target.kind === "inline" - ? { - kind: "function" as const, - functionType: "mutation" as const, - handle: "inline", - ...commonFields, - } - : target.kind === "function" - ? { - kind: "function" as const, - functionType: target.functionType, - handle: await createFunctionHandle(target.function), - ...commonFields, - } - : target.kind === "workflow" - ? { - kind: "workflow" as const, - handle: await createFunctionHandle(target.function), - ...commonFields, - } - : target.kind === "event" - ? { - kind: "event" as const, - eventId: target.args.eventId, - ...commonFields, - args: target.args, - } - : target.kind === "sleep" - ? { - kind: "sleep" as const, - ...commonFields, - } - : ((): never => { - throw new Error( - `Unknown step kind: ${(target as any).kind}`, - ); - })(); + const step = await buildStep(target, commonFields); // Helper function (outside the map callback): async function buildStep( target: StepRequest<DataModel>["target"], commonFields: Omit<Step, "kind"> ): Promise<Step> { switch (target.kind) { case "inline": return { kind: "function", functionType: "mutation", handle: "inline", ...commonFields, }; case "function": return { kind: "function", functionType: target.functionType, handle: await createFunctionHandle(target.function), ...commonFields, }; case "workflow": return { kind: "workflow", handle: await createFunctionHandle(target.function), ...commonFields, }; case "event": return { kind: "event", eventId: target.args.eventId, ...commonFields, args: target.args, }; case "sleep": return { kind: "sleep", ...commonFields, }; default: throw new Error(`Unknown step kind: ${(target as never)}`); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/step.ts` around lines 218 - 255, The nested ternary that builds step is hard to read; refactor the logic that constructs step (currently using target.kind and createFunctionHandle/commonFields) into a clearer switch or helper function (e.g., buildStepFromTarget) that handles cases "inline" (set kind:"function", functionType:"mutation", handle:"inline"), "function" (use createFunctionHandle(target.function) and target.functionType), "workflow" (use createFunctionHandle(target.function)), "event" (copy eventId and args), and "sleep" (copy commonFields), and keep the exhaustive fallback that throws new Error(`Unknown step kind: ${(target as any).kind}`) so the compile-time never-check remains; ensure all references to createFunctionHandle, commonFields, step, and target.kind are preserved exactly.
🤖 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/eventTimeout.ts`:
- Around line 44-53: The inline step runs are closing over eventId and
scheduledFnId but journal an empty deps object, risking reuse of stale inline
entries; update both step.run calls (the scheduling step that calls
ctx.scheduler.runAfter with internal.eventTimeout.timeoutEvent and the
cancellation step that uses scheduledFnId) to pass deps that include the
closed-over values (e.g., deps: { eventId } for the scheduler step and deps: {
eventId, scheduledFnId } for the cancel step) so replay/restart correctly
distinguishes entries and schedules/cancels the right timeout.
- Around line 110-129: startEventTimeout currently returns only the WorkflowId
which prevents the CLI approve step from knowing the approval eventId; update
startEventTimeout (the internalMutation that calls workflow.start with
internal.eventTimeout.eventTimeoutWorkflow) to also expose or persist the
approval event ID: either have the workflow return the approvalEventId as part
of the start result and return { workflowId, approvalEventId } from
startEventTimeout, or persist the generated approvalEventId in a DB/table keyed
by workflowId inside the workflow and add a small lookup helper function (e.g.
getApprovalEventIdByWorkflowId) that the CLI can call to retrieve the eventId by
workflowId. Ensure the change references startEventTimeout and
internal.eventTimeout.eventTimeoutWorkflow so callers can obtain the eventId for
the approve command.
In `@src/client/stepContext.test.ts`:
- Around line 349-415: The replay helper replayFromJournal is only feeding back
runResult and must be extended to validate that the StepRequest emitted by
ctx.run matches the journal entry metadata (name, functionType, handle, deps,
args) the same way StepExecutor.completeMessage builds its message; update
replayFromJournal to capture the outgoing StepRequest from the BaseChannel (or
the function that sends messages during createWorkflowCtx) and assert each field
(entry.name, entry.functionType, entry.handle, entry.deps, entry.args) equals
the corresponding properties on the emitted request before returning the
runResult, so tests for custom name/functionType/handle/deps fail if ctx.run
serializes the wrong request shape.
---
Nitpick comments:
In `@example/convex/example.ts`:
- Around line 53-63: The inline handler closes over step.workflowId and computed
weather fields but writes an empty journaled object, so changes to those
captured values won’t be detected on replay; fix it by capturing the needed
values into locals before calling step.run (e.g., const capturedWorkflowId =
step.workflowId; const capturedOut = { name, celsius, farenheit, windSpeed,
windGust }) or by passing them into the step.run closure, and then include those
captured values in the ctx.db.patch call (set out to the capturedOut and/or
include capturedWorkflowId) so step.run, ctx.db.query("flows").withIndex(...),
and ctx.db.patch persist the actual captured inputs for proper journaling and
replay detection.
In `@src/client/step.ts`:
- Around line 218-255: The nested ternary that builds step is hard to read;
refactor the logic that constructs step (currently using target.kind and
createFunctionHandle/commonFields) into a clearer switch or helper function
(e.g., buildStepFromTarget) that handles cases "inline" (set kind:"function",
functionType:"mutation", handle:"inline"), "function" (use
createFunctionHandle(target.function) and target.functionType), "workflow" (use
createFunctionHandle(target.function)), "event" (copy eventId and args), and
"sleep" (copy commonFields), and keep the exhaustive fallback that throws new
Error(`Unknown step kind: ${(target as any).kind}`) so the compile-time
never-check remains; ensure all references to createFunctionHandle,
commonFields, step, and target.kind are preserved exactly.
🪄 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: a53a0ada-2000-444a-98f4-7270f6e9e65d
⛔ Files ignored due to path filters (1)
example/convex/_generated/api.d.tsis excluded by!**/_generated/**
📒 Files selected for processing (9)
example/convex/eventTimeout.tsexample/convex/example.tssrc/client/index.tssrc/client/step.tssrc/client/stepContext.test.tssrc/client/workflowContext.tssrc/client/workflowMutation.tssrc/component/journal.tssrc/component/workflow.ts
Comment on lines
+44
to
+53
| const scheduledFnId = await step.run( | ||
| async (ctx) => { | ||
| return ctx.scheduler.runAfter( | ||
| 30_000, | ||
| internal.eventTimeout.timeoutEvent, | ||
| { eventId }, | ||
| ); | ||
| }, | ||
| { name: "scheduleTimeout" }, | ||
| ); |
There was a problem hiding this comment.
Journal the captured IDs as deps in both inline steps.
Both handlers close over eventId / scheduledFnId but currently journal {}. If this workflow is replayed or restarted before those steps, the old inline entries can be reused for a different event or scheduled function, so the timeout won’t be scheduled or canceled correctly.
Suggested change
const scheduledFnId = await step.run(
async (ctx) => {
return ctx.scheduler.runAfter(
30_000,
internal.eventTimeout.timeoutEvent,
{ eventId },
);
},
- { name: "scheduleTimeout" },
+ {
+ name: "scheduleTimeout",
+ deps: { eventId },
+ },
);
@@
await step.run(
async (ctx) => {
const scheduled = await ctx.db.system.get(scheduledFnId);
if (scheduled?.state.kind === "pending")
await ctx.scheduler.cancel(scheduledFnId);
},
- { name: "cancelTimeout" },
+ {
+ name: "cancelTimeout",
+ deps: { scheduledFnId },
+ },
);Also applies to: 60-67
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@example/convex/eventTimeout.ts` around lines 44 - 53, The inline step runs
are closing over eventId and scheduledFnId but journal an empty deps object,
risking reuse of stale inline entries; update both step.run calls (the
scheduling step that calls ctx.scheduler.runAfter with
internal.eventTimeout.timeoutEvent and the cancellation step that uses
scheduledFnId) to pass deps that include the closed-over values (e.g., deps: {
eventId } for the scheduler step and deps: { eventId, scheduledFnId } for the
cancel step) so replay/restart correctly distinguishes entries and
schedules/cancels the right timeout.
ianmacartney
force-pushed
the
ian/inline-ctx-run
branch
from
8fc81b8 to
e271b53
Compare
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 `@src/client/workflowMutation.ts`:
- Around line 8-11: The import list in workflowMutation.ts includes an unused
symbol GenericMutationCtx; remove GenericMutationCtx from the named imports
(leaving GenericDataModel and RegisteredMutation) to clean up the imports and
satisfy static analysis tools, then run type checks to ensure no usages remain
of GenericMutationCtx in functions or signatures within this file (e.g.,
references in any mutation-related types or functions).
🪄 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: cbaebcc1-2ccd-4701-97ff-c0007ff80714
⛔ Files ignored due to path filters (1)
example/convex/_generated/api.d.tsis excluded by!**/_generated/**
📒 Files selected for processing (9)
example/convex/eventTimeout.tsexample/convex/example.tssrc/client/index.tssrc/client/step.tssrc/client/stepContext.test.tssrc/client/workflowContext.tssrc/client/workflowMutation.tssrc/component/journal.tssrc/component/workflow.ts
✅ Files skipped from review due to trivial changes (1)
- src/component/workflow.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/component/journal.ts
- src/client/index.ts
ianmacartney
force-pushed
the
ian/inline-ctx-run
branch
from
e271b53 to
654fa8a
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
example/convex/eventTimeout.ts (2)
44-53: Consider addingdepsfor the closed-overeventId.The handler closes over
eventIdfrom the outer scope. While inline steps journal their results and skip re-execution on replay, providingdeps: { eventId }would help detect workflow code changes that alter the dependency chain and trigger appropriate re-execution if values don't match.This is optional since
eventIdis deterministically produced by the prior step within the same workflow run, but it documents the dependency and enables mismatch detection if the workflow logic changes.Suggested change
const scheduledFnId = await step.run( async (ctx) => { return ctx.scheduler.runAfter( 30_000, internal.eventTimeout.timeoutEvent, { eventId }, ); }, - { name: "scheduleTimeout" }, + { name: "scheduleTimeout", deps: { eventId } }, );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@example/convex/eventTimeout.ts` around lines 44 - 53, The inline step passed to step.run closes over eventId; add deps: { eventId } to the step.run options so replay detects dependency mismatches—i.e. when scheduling with step.run(async (ctx) => ctx.scheduler.runAfter(..., internal.eventTimeout.timeoutEvent, { eventId }), { name: "scheduleTimeout", deps: { eventId } }) ensure the closed-over eventId is declared as a dependency to trigger re-execution if it changes.
60-67: Consider addingdepsfor the closed-overscheduledFnId.Similar to the scheduling step, this handler closes over
scheduledFnId. Addingdeps: { scheduledFnId }documents the dependency and enables journal mismatch detection.Suggested change
await step.run( async (ctx) => { const scheduled = await ctx.db.system.get(scheduledFnId); if (scheduled?.state.kind === "pending") await ctx.scheduler.cancel(scheduledFnId); }, - { name: "cancelTimeout" }, + { name: "cancelTimeout", deps: { scheduledFnId } }, );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@example/convex/eventTimeout.ts` around lines 60 - 67, The cancelTimeout step.run handler closes over scheduledFnId but doesn't declare it in deps; update the step.run call for the "cancelTimeout" step to include deps: { scheduledFnId } so the scheduler/journal can detect mismatches — i.e., add the deps object to the options passed to step.run (the same place where { name: "cancelTimeout" } is set) referencing the scheduledFnId variable.src/client/index.ts (1)
600-600: MissingDataModelgeneric onWorkflowreturn type.The second
defineoverload at line 600 returnsWorkflow<ArgsValidator, ReturnsValidator>without theDataModelgeneric, whiledefineWorkflowat line 190 returnsWorkflow<AV, RV, DM>. This inconsistency means workflows defined viamanager.define({...}).withHandlerRef(...)will lose theDataModeltyping.Suggested change
withHandlerRef( ref: FunctionReference< "mutation", "internal", WorkflowArgs<ArgsValidator>, ReturnValueForOptionalValidator<ReturnsValidator> >, - ): Workflow<ArgsValidator, ReturnsValidator>; + ): Workflow<ArgsValidator, ReturnsValidator, DataModel>;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/index.ts` at line 600, The second overload of define returns Workflow<ArgsValidator, ReturnsValidator> without the DataModel generic causing loss of DM typing; update that overload signature to return Workflow<ArgsValidator, ReturnsValidator, DataModel> (matching defineWorkflow at line 190) so manager.define({...}).withHandlerRef(...) retains the DataModel generic; ensure the generic name matches the overload's type parameter (DataModel) and adjust any related type parameter list on the define overload if necessary.
🤖 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/eventTimeout.ts`:
- Around line 110-129: The CLI can't approve because startEventTimeout (which
calls workflow.start with internal.eventTimeout.eventTimeoutWorkflow) only
returns a WorkflowId while the workflow generates the eventId internally;
persist the eventId keyed by the workflowId so callers can look it up. Update
the eventTimeoutWorkflow handler to write the generated eventId into a small
table (e.g., EventApproval table) keyed by the WorkflowId returned by
workflow.start, and update startEventTimeout to return the WorkflowId
(unchanged) while adding a helper query (or document) that reads
EventApproval[workflowId] to get the eventId; reference startEventTimeout,
workflow.start, internal.eventTimeout.eventTimeoutWorkflow, and the new
EventApproval mapping in your changes.
---
Nitpick comments:
In `@example/convex/eventTimeout.ts`:
- Around line 44-53: The inline step passed to step.run closes over eventId; add
deps: { eventId } to the step.run options so replay detects dependency
mismatches—i.e. when scheduling with step.run(async (ctx) =>
ctx.scheduler.runAfter(..., internal.eventTimeout.timeoutEvent, { eventId }), {
name: "scheduleTimeout", deps: { eventId } }) ensure the closed-over eventId is
declared as a dependency to trigger re-execution if it changes.
- Around line 60-67: The cancelTimeout step.run handler closes over
scheduledFnId but doesn't declare it in deps; update the step.run call for the
"cancelTimeout" step to include deps: { scheduledFnId } so the scheduler/journal
can detect mismatches — i.e., add the deps object to the options passed to
step.run (the same place where { name: "cancelTimeout" } is set) referencing the
scheduledFnId variable.
In `@src/client/index.ts`:
- Line 600: The second overload of define returns Workflow<ArgsValidator,
ReturnsValidator> without the DataModel generic causing loss of DM typing;
update that overload signature to return Workflow<ArgsValidator,
ReturnsValidator, DataModel> (matching defineWorkflow at line 190) so
manager.define({...}).withHandlerRef(...) retains the DataModel generic; ensure
the generic name matches the overload's type parameter (DataModel) and adjust
any related type parameter list on the define overload if necessary.
🪄 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: 6a2b53b9-a742-4233-afcb-7fadc7ad44d1
⛔ Files ignored due to path filters (1)
example/convex/_generated/api.d.tsis excluded by!**/_generated/**
📒 Files selected for processing (9)
example/convex/eventTimeout.tsexample/convex/example.tssrc/client/index.tssrc/client/step.tssrc/client/stepContext.test.tssrc/client/workflowContext.tssrc/client/workflowMutation.tssrc/component/journal.tssrc/component/workflow.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- src/component/workflow.ts
- src/component/journal.ts
- src/client/stepContext.test.ts
- src/client/workflowMutation.ts
Comment on lines
+110
to
+129
| /** | ||
| * Test this from the CLI: | ||
| * ```sh | ||
| * npx convex run eventTimeout:startEventTimeout | ||
| * ``` | ||
| * Then either approve before 30s: | ||
| * ```sh | ||
| * npx convex run eventTimeout:approve '{"eventId":"..."}' | ||
| * ``` | ||
| * Or wait 30s for the timeout to fire automatically. | ||
| */ | ||
| export const startEventTimeout = internalMutation({ | ||
| args: {}, | ||
| handler: async (ctx): Promise<WorkflowId> => { | ||
| return await workflow.start( | ||
| ctx, | ||
| internal.eventTimeout.eventTimeoutWorkflow, | ||
| {}, | ||
| ); | ||
| }, |
There was a problem hiding this comment.
CLI example cannot be run as documented.
The docstring instructs users to call approve '{"eventId":"..."}', but startEventTimeout only returns the WorkflowId. The eventId is created inside the workflow handler and is never exposed to the caller.
Consider either:
- Persisting the
eventIdto a table keyed byworkflowIdso users can look it up - Returning both
workflowIdandeventIdfrom the workflow's first step - Adding a helper query that retrieves the approval event for a given workflow
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@example/convex/eventTimeout.ts` around lines 110 - 129, The CLI can't approve
because startEventTimeout (which calls workflow.start with
internal.eventTimeout.eventTimeoutWorkflow) only returns a WorkflowId while the
workflow generates the eventId internally; persist the eventId keyed by the
workflowId so callers can look it up. Update the eventTimeoutWorkflow handler to
write the generated eventId into a small table (e.g., EventApproval table) keyed
by the WorkflowId returned by workflow.start, and update startEventTimeout to
return the WorkflowId (unchanged) while adding a helper query (or document) that
reads EventApproval[workflowId] to get the eventId; reference startEventTimeout,
workflow.start, internal.eventTimeout.eventTimeoutWorkflow, and the new
EventApproval mapping in your changes.
a89e690 to
49cd8ac
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/client/index.ts (1)
589-596:⚠️ Potential issue | 🟠 MajorAdd
DataModelas third generic parameter inwithHandlerRef()return type.The
withHandlerRef()overload at line 97 returnsWorkflow<ArgsValidator, ReturnsValidator>, omitting theDataModelgeneric parameter. SinceWorkflowis defined asWorkflow<AV, RV, DM>and itshandler()method signature usesstep: WorkflowCtx<DM>, defaultingDMtoGenericDataModelbreaks typed access toctx.dbin handlers. TheDataModelfrom theWorkflowManager<DataModel>instance should be threaded through this path, just as it is in thedefineWorkflow()call at line 125.Proposed fix
- ): Workflow<ArgsValidator, ReturnsValidator>; + ): Workflow<ArgsValidator, ReturnsValidator, DataModel>;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/index.ts` around lines 589 - 596, The overload for withHandlerRef(...) currently returns Workflow<ArgsValidator, ReturnsValidator> which omits the DataModel generic and causes handlers to default to GenericDataModel; update the return type to include the third generic (e.g., Workflow<ArgsValidator, ReturnsValidator, DataModel>) and ensure the DataModel from the surrounding WorkflowManager<DataModel> is threaded through this overload so handler() receives WorkflowCtx<DataModel> (match the same generic threading used by defineWorkflow()).
♻️ Duplicate comments (4)
example/convex/eventTimeout.ts (2)
45-68:⚠️ Potential issue | 🟠 MajorJournal the captured IDs in both inline handlers.
Both
step.run()calls close over values from the outer scope, but both currently journal{}. Replays can therefore reuse stale inline entries and schedule/cancel the wrong timeout job.Proposed fix
const scheduledFnId = await step.run( async (ctx) => { return ctx.scheduler.runAfter( 30_000, internal.eventTimeout.timeoutEvent, { eventId }, ); }, - { name: "scheduleTimeout" }, + { + name: "scheduleTimeout", + deps: { eventId }, + }, ); @@ await step.run( async (ctx) => { const scheduled = await ctx.db.system.get(scheduledFnId); if (scheduled?.state.kind === "pending") await ctx.scheduler.cancel(scheduledFnId); }, - { name: "cancelTimeout" }, + { + name: "cancelTimeout", + deps: { scheduledFnId }, + }, );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@example/convex/eventTimeout.ts` around lines 45 - 68, The inline step.run handlers close over outer-scope IDs causing journals to record {} and risk replaying stale entries; update the two inline handlers used with step.run (the scheduler call that creates scheduledFnId via ctx.scheduler.runAfter and the cancel handler that uses ctx.scheduler.cancel) to explicitly journal the captured IDs (scheduledFnId and eventId) so replays record and reuse the correct identifiers when running or canceling the timeout job; ensure the handler that schedules the timeout writes the generated scheduledFnId and eventId into the journal and the cancel handler reads/journals those same IDs before calling ctx.scheduler.cancel.
111-129:⚠️ Potential issue | 🟡 MinorThe CLI example still can't approve as written.
startEventTimeout()only returns theWorkflowId, but Line 118 requires aneventIdthat this module never persists or exposes. Users following the docstring have no way to run the approval command successfully.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@example/convex/eventTimeout.ts` around lines 111 - 129, The CLI example is broken because startEventTimeout (exported function startEventTimeout) only returns a WorkflowId while the CLI approve command expects an eventId that is never persisted or exposed; modify startEventTimeout (the internalMutation handler that calls workflow.start with internal.eventTimeout.eventTimeoutWorkflow) to persist or expose the eventId generated by the workflow and return it to the caller (or log it to a known location) so users can run the approve command, e.g., have the workflow produce/return an eventId and change startEventTimeout to persist that id or include it in the response instead of only returning WorkflowId.src/client/workflowContext.ts (1)
189-229:⚠️ Potential issue | 🟠 MajorDon't use a shared
inlineDepthcounter for this guard.This flag is attached to the whole
WorkflowCtx, so one inline handler can block unrelated sibling work running on another async branch. A pattern likePromise.all([step.run(async () => await gate), gate.then(() => step.runMutation(...))])can now fail even though the mutation call is outside the inline handler. The guard needs async-call-chain scoping, not instance-wide state. Please verify against a concurrent sibling case.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/client/workflowContext.ts` around lines 189 - 229, The guard currently uses a shared inlineDepth counter (inlineDepth) which wrongly blocks unrelated async branches; replace it with an async-call-chain scoped counter using Node's AsyncLocalStorage (or equivalent) so the depth is tracked per async context. Specifically, replace inlineDepth and guardNotInlined with an AsyncLocalStorage-based store (create an AsyncLocalStorage<number> named e.g. inlineDepthStore), change guardNotInlined to read inlineDepthStore.getStore() (treat undefined as 0) and throw only if that value > 0, and when you invoke the inline handler inside run (the handler passed to run(sender, { target: { kind: "inline", handler: ... } })), wrap the handler execution using inlineDepthStore.run(currentDepth+1, async () => handler(ctx)) so the increment is local to that async call chain and decremented automatically when the context ends.example/convex/example.ts (1)
57-67:⚠️ Potential issue | 🟠 MajorJournal the captured values in this inline step.
This handler closes over
step.workflowIdand the computed weather payload, but it currently journals{}. That lets replay reuse a stale inline entry even when those captured values changed, which breaks the determinism this new API is trying to demonstrate.Proposed fix
- await step.run(async (ctx) => { - const flow = await ctx.db - .query("flows") - .withIndex("workflowId", (q) => q.eq("workflowId", step.workflowId)) - .first(); - if (flow) { - await ctx.db.patch(flow._id, { - out: { name, celsius, farenheit, windSpeed, windGust }, - }); - } - }); + await step.run( + async (ctx) => { + const flow = await ctx.db + .query("flows") + .withIndex("workflowId", (q) => q.eq("workflowId", step.workflowId)) + .first(); + if (flow) { + await ctx.db.patch(flow._id, { + out: { name, celsius, farenheit, windSpeed, windGust }, + }); + } + }, + { + name: "updateFlow", + deps: { + workflowId: step.workflowId, + name, + celsius, + farenheit, + windSpeed, + windGust, + }, + }, + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@example/convex/example.ts` around lines 57 - 67, The inline step handler currently journals an empty object which allows replay to reuse stale entries; update the handler inside step.run to call step.journal with the captured values so they are persisted deterministically. Specifically, inside the async callback passed to step.run (the block that queries "flows" and calls ctx.db.patch(flow._id,...)), invoke step.journal({ workflowId: step.workflowId, name, celsius, farenheit, windSpeed, windGust }) (or an equivalent object containing the workflowId and the computed out payload) before or after the ctx.db.patch call so replay uses the exact captured values.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/client/index.ts`:
- Around line 589-596: The overload for withHandlerRef(...) currently returns
Workflow<ArgsValidator, ReturnsValidator> which omits the DataModel generic and
causes handlers to default to GenericDataModel; update the return type to
include the third generic (e.g., Workflow<ArgsValidator, ReturnsValidator,
DataModel>) and ensure the DataModel from the surrounding
WorkflowManager<DataModel> is threaded through this overload so handler()
receives WorkflowCtx<DataModel> (match the same generic threading used by
defineWorkflow()).
---
Duplicate comments:
In `@example/convex/eventTimeout.ts`:
- Around line 45-68: The inline step.run handlers close over outer-scope IDs
causing journals to record {} and risk replaying stale entries; update the two
inline handlers used with step.run (the scheduler call that creates
scheduledFnId via ctx.scheduler.runAfter and the cancel handler that uses
ctx.scheduler.cancel) to explicitly journal the captured IDs (scheduledFnId and
eventId) so replays record and reuse the correct identifiers when running or
canceling the timeout job; ensure the handler that schedules the timeout writes
the generated scheduledFnId and eventId into the journal and the cancel handler
reads/journals those same IDs before calling ctx.scheduler.cancel.
- Around line 111-129: The CLI example is broken because startEventTimeout
(exported function startEventTimeout) only returns a WorkflowId while the CLI
approve command expects an eventId that is never persisted or exposed; modify
startEventTimeout (the internalMutation handler that calls workflow.start with
internal.eventTimeout.eventTimeoutWorkflow) to persist or expose the eventId
generated by the workflow and return it to the caller (or log it to a known
location) so users can run the approve command, e.g., have the workflow
produce/return an eventId and change startEventTimeout to persist that id or
include it in the response instead of only returning WorkflowId.
In `@example/convex/example.ts`:
- Around line 57-67: The inline step handler currently journals an empty object
which allows replay to reuse stale entries; update the handler inside step.run
to call step.journal with the captured values so they are persisted
deterministically. Specifically, inside the async callback passed to step.run
(the block that queries "flows" and calls ctx.db.patch(flow._id,...)), invoke
step.journal({ workflowId: step.workflowId, name, celsius, farenheit, windSpeed,
windGust }) (or an equivalent object containing the workflowId and the computed
out payload) before or after the ctx.db.patch call so replay uses the exact
captured values.
In `@src/client/workflowContext.ts`:
- Around line 189-229: The guard currently uses a shared inlineDepth counter
(inlineDepth) which wrongly blocks unrelated async branches; replace it with an
async-call-chain scoped counter using Node's AsyncLocalStorage (or equivalent)
so the depth is tracked per async context. Specifically, replace inlineDepth and
guardNotInlined with an AsyncLocalStorage-based store (create an
AsyncLocalStorage<number> named e.g. inlineDepthStore), change guardNotInlined
to read inlineDepthStore.getStore() (treat undefined as 0) and throw only if
that value > 0, and when you invoke the inline handler inside run (the handler
passed to run(sender, { target: { kind: "inline", handler: ... } })), wrap the
handler execution using inlineDepthStore.run(currentDepth+1, async () =>
handler(ctx)) so the increment is local to that async call chain and decremented
automatically when the context ends.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ddac9ece-0906-4bb8-9a2d-fee8ed09b58d
⛔ Files ignored due to path filters (1)
example/convex/_generated/api.d.tsis excluded by!**/_generated/**
📒 Files selected for processing (9)
example/convex/eventTimeout.tsexample/convex/example.tssrc/client/index.tssrc/client/step.tssrc/client/stepContext.test.tssrc/client/workflowContext.tssrc/client/workflowMutation.tssrc/component/journal.tssrc/component/workflow.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- src/component/workflow.ts
- src/component/journal.ts
- src/client/workflowMutation.ts
- src/client/stepContext.test.ts
ianmacartney
force-pushed
the
ian/inline-ctx-run
branch
from
49cd8ac to
f222a94
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Add inline handler support to workflow context
This change introduces a new
ctx.run()method that allows executing inline handlers within workflow mutation transactions. The handler receives the mutation context and can perform database operations, with results being journaled for replay consistency.Key Changes
ctx.run()method toWorkflowCtxthat executes inline handlers with access to the mutation contextinternalMutationparameter to workflow definitions for fully typed database accessDataModeltype parameter toWorkflowDefinitionandWorkflowCtxfor type safetyThe inline handlers are journaled like other workflow steps, ensuring deterministic replay behavior while providing direct access to the underlying mutation transaction.
Summary by CodeRabbit
New Features
Tests