Skip to content

Require both onComplete and context or neither#253

Merged
ianmacartney merged 2 commits intomainfrom
issue-252
May 5, 2026
Merged

Require both onComplete and context or neither#253
ianmacartney merged 2 commits intomainfrom
issue-252

Conversation

@ianmacartney
Copy link
Copy Markdown
Member

@ianmacartney ianmacartney commented May 5, 2026

Fixes #252

This PR adds tests verifying that the context value passed to onComplete is correctly round-tripped through workflow failure paths — both when the workflow handler throws and when it returns an oversized result — covering both the direct mutation call and workflow.start() invocation styles.

It also tightens the CallbackOptions and WorkflowArgs types so that onComplete and context must be provided together or not at all, using a discriminated union. This prevents callers from supplying one without the other, which was previously allowed but semantically invalid.

Copy link
Copy Markdown
Member Author

This stack of pull requests is managed by Graphite. Learn more about stacking.

@pkg-pr-new
Copy link
Copy Markdown

pkg-pr-new Bot commented May 5, 2026

Open in StackBlitz

npm i https://pkg.pr.new/@convex-dev/workflow@253

commit: 2de18bc

@ianmacartney ianmacartney changed the title Require both onComplete and context together or neither Require both onComplete and context or neither May 5, 2026
@ianmacartney ianmacartney marked this pull request as ready for review May 5, 2026 01:22
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 5, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository: get-convex/coderabbit/.coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 58311878-eb3f-4c5d-b333-793fddf07a2a

📥 Commits

Reviewing files that changed from the base of the PR and between 0179b44 and 2de18bc.

⛔ Files ignored due to path filters (1)
  • example/convex/_generated/api.d.ts is excluded by !**/_generated/**
📒 Files selected for processing (4)
  • example/convex/test/contextRoundtrip.test.ts
  • example/convex/test/contextRoundtrip.ts
  • src/client/index.ts
  • src/client/workflowMutation.ts

📝 Walkthrough

Walkthrough

The PR refactors the type contracts for workflow callback options from optional individual fields to discriminated unions. CallbackOptions and WorkflowArgs are updated to enforce that when onComplete is provided, context must also be provided, or neither should be present. This change pairs with new test coverage validating that workflow execution preserves context through failure scenarios when callbacks are captured via both direct invocation and workflow start paths.

Possibly related PRs

  • get-convex/workflow#244: Modifies the same client-level workflow start types and onComplete/context handling in src/client/index.ts and src/client/workflowMutation.ts.
  • get-convex/workflow#222: Modifies workflow invocation and completion behavior, touching src/client/workflowMutation.ts and adding tests around workflow failure and context capture.
  • get-convex/workflow#251: Re-exports WorkflowArgs from workflowMutation, directly affected by the discriminated union type changes in this PR.

Suggested reviewers

  • reeceyang
  • goffrie
🚥 Pre-merge checks | ✅ 3 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Linked Issues check ⚠️ Warning The PR partially addresses issue #252 by tightening types with a discriminated union, but does not implement the core fix: making context optional in OnCompleteArgs as specified. The tests verify round-tripping but the runtime omission issue remains unresolved. Update OnCompleteArgs to declare context?: unknown as optional to match the runtime behavior on failure paths, allowing the type system to be consistent with actual execution.
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately and concisely describes the main change: enforcing that onComplete and context must be provided together or neither, which is reflected in the discriminated union type updates in the codebase.
Description check ✅ Passed The description clearly explains both the test additions for context round-tripping through failure paths and the type-level changes using discriminated unions, directly addressing issue #252.
Out of Scope Changes check ✅ Passed All changes are directly scoped to issue #252: test files verify context round-tripping, and type changes in CallbackOptions and WorkflowArgs enforce the onComplete/context pairing requirement.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch issue-252

Tip

💬 Introducing Slack Agent: The best way for teams to turn conversations into code.

Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.

  • Generate code and open pull requests
  • Plan features and break down work
  • Investigate incidents and troubleshoot customer tickets together
  • Automate recurring tasks and respond to alerts with triggers
  • Summarize progress and report instantly

Built for teams:

  • Shared memory across your entire org—no repeating context
  • Per-thread sandboxes to safely plan and execute work
  • Governance built-in—scoped access, auditability, and budget controls

One agent for your entire SDLC. Right inside Slack.

👉 Get started


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@ianmacartney ianmacartney requested a review from reeceyang May 5, 2026 19:34
@ianmacartney ianmacartney merged commit 2b906ba into main May 5, 2026
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

OnCompleteArgs.context typed as required, but runtime omits it on the failure path

2 participants