Added no op event to workflow runner #2884
Conversation
|
WalkthroughThe changes in this pull request encompass updates to multiple projects, primarily involving version increments and dependency updates. Notably, the Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
packages/workflow-core/src/lib/workflow-runner.ts (1)
544-550: Enhance logging context for better debugging.The logging implementation could be more informative by including additional context that would be helpful during debugging.
Consider enhancing the log message:
- const noOp = () => { - logger.log(`${BUILT_IN_EVENT.NO_OP} action fired`, { - state - }) - } + const noOp = (context: Record<string, unknown>, event: Record<PropertyKey, unknown>) => { + logger.log(`${BUILT_IN_EVENT.NO_OP} action fired`, { + state, + workflowId: this.#__runtimeId, + eventType: event.type, + contextKeys: Object.keys(context) + }) + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (18)
apps/backoffice-v2/CHANGELOG.md(1 hunks)apps/backoffice-v2/package.json(2 hunks)apps/kyb-app/CHANGELOG.md(1 hunks)apps/kyb-app/package.json(2 hunks)examples/headless-example/CHANGELOG.md(1 hunks)examples/headless-example/package.json(2 hunks)packages/workflow-core/CHANGELOG.md(1 hunks)packages/workflow-core/package.json(1 hunks)packages/workflow-core/src/lib/built-in-event.ts(1 hunks)packages/workflow-core/src/lib/utils/definition-validator/states-validator.ts(3 hunks)packages/workflow-core/src/lib/workflow-runner.ts(2 hunks)sdks/workflow-browser-sdk/CHANGELOG.md(1 hunks)sdks/workflow-browser-sdk/package.json(2 hunks)sdks/workflow-node-sdk/CHANGELOG.md(1 hunks)sdks/workflow-node-sdk/package.json(2 hunks)services/workflows-service/CHANGELOG.md(1 hunks)services/workflows-service/package.json(2 hunks)services/workflows-service/prisma/data-migrations(1 hunks)
✅ Files skipped from review due to trivial changes (13)
- packages/workflow-core/package.json
- sdks/workflow-browser-sdk/package.json
- services/workflows-service/prisma/data-migrations
- apps/kyb-app/CHANGELOG.md
- examples/headless-example/package.json
- examples/headless-example/CHANGELOG.md
- sdks/workflow-node-sdk/package.json
- services/workflows-service/CHANGELOG.md
- apps/backoffice-v2/CHANGELOG.md
- sdks/workflow-node-sdk/CHANGELOG.md
- apps/kyb-app/package.json
- sdks/workflow-browser-sdk/CHANGELOG.md
- apps/backoffice-v2/package.json
🔇 Additional comments (9)
packages/workflow-core/src/lib/utils/definition-validator/states-validator.ts (2)
3-4: Import statements updated appropriately
The imports for AnyRecord, isObject, and BUILT_IN_EVENT have been added to support the new validation logic.
13-15: Extended TTransitionOption to include actions property
The TTransitionOption type now includes an object with an actions property, allowing transitions without a target state, such as no-op transitions.
packages/workflow-core/src/lib/built-in-event.ts (1)
4-4: Added NO_OP to BUILT_IN_EVENT
The NO_OP constant has been correctly added to BUILT_IN_EVENT, expanding the set of built-in events to include a no-operation action.
services/workflows-service/package.json (3)
4-4: Version number updated
The version has been incremented to "0.7.80", reflecting the new changes in this release.
42-42: Updated db:data-migration:migrate script to ensure a clean build
The script now includes:
- Clearing the
./distdirectory withrimrafto prevent stale artifacts. - Building the project before running the migration script.
- Starting the NestJS application with the specified entry file.
This ensures that the migration runs against the latest build.
53-54: Dependencies updated to latest versions
Updated:
"@ballerine/workflow-core"to"0.6.76""@ballerine/workflow-node-sdk"to"0.6.76"
These updates ensure compatibility with the latest features and fixes.
packages/workflow-core/CHANGELOG.md (1)
3-8: Changelog updated for version 0.6.76
The changelog correctly documents the addition of the no-op event to the workflow runner:
- Patch Changes
- Added no op event to workflow runner
This provides transparency and keeps users informed about the new feature.
packages/workflow-core/src/lib/workflow-runner.ts (2)
562-564: LGTM: Clean integration of the NO_OP event handler.
The implementation follows the established pattern for built-in events and is properly integrated into the state machine configuration.
562-564: Verify BUILT_IN_EVENT constant definition.
Let's ensure the NO_OP event is properly defined in the constants file.
✅ Verification successful
BUILT_IN_EVENT.NO_OP is properly defined and used correctly
The verification confirms that:
BUILT_IN_EVENT.NO_OPis properly defined inpackages/workflow-core/src/lib/built-in-event.ts- The constant is correctly imported and used in
workflow-runner.ts - The implementation follows the same pattern as other built-in events (UPDATE_CONTEXT, DEEP_MERGE_CONTEXT)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if NO_OP is defined in built-in-event.ts
rg -A 1 "export const BUILT_IN_EVENT = {"
Length of output: 169
Script:
#!/bin/bash
# Search for BUILT_IN_EVENT constant definition with proper escaping
rg "BUILT_IN_EVENT" -A 3
Length of output: 19980
packages/workflow-core/src/lib/utils/definition-validator/states-validator.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
packages/workflow-core/src/lib/constants.ts(1 hunks)packages/workflow-core/src/lib/workflow-runner.ts(3 hunks)
🔇 Additional comments (2)
packages/workflow-core/src/lib/constants.ts (1)
44-49: LGTM! Well-structured constant and type definition.
The new BUILT_IN_ACTION constant and BuiltInAction type are well-defined, following TypeScript best practices and maintaining consistency with the codebase's naming conventions.
packages/workflow-core/src/lib/workflow-runner.ts (1)
7-7: LGTM! Clean implementation of the no-op action.
The no-operation action is well-integrated into the workflow runner's action system, following the existing patterns and maintaining consistency with the codebase.
Also applies to: 472-476
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
packages/workflow-core/src/lib/built-in-action.ts (1)
1-5: LGTM! Consider adding JSDoc documentation.The implementation is clean and follows TypeScript best practices with proper type safety using const assertions.
Consider adding JSDoc documentation to describe the purpose and usage of the NO_OP action:
+/** + * Built-in actions available in the workflow runner. + * @const {Object} + */ export const BUILT_IN_ACTION = { + /** + * No-operation action that can be used as a placeholder or to explicitly indicate no action is needed. + */ NO_OP: 'NO_OP', } as const;packages/workflow-core/src/lib/plugins/external-plugin/email-plugin.ts (2)
60-60: Consider using a more type-safe approach for headers mockWhile adding headers maintains interface consistency, using type assertion (
as Headers) might hide potential type issues. Consider creating a proper Headers instance instead.-headers: {} as Headers, +headers: new Headers(),
Line range hint
1-71: Consider improving email provider abstractionThe current implementation tightly couples the email logic with SendGrid. Consider:
- Moving the adapter selection to configuration
- Creating separate adapter classes for different providers
- Using dependency injection for better testability
This would make it easier to:
- Switch email providers
- Test the email functionality
- Add new providers in the future
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
packages/workflow-core/src/index.ts(1 hunks)packages/workflow-core/src/lib/built-in-action.ts(1 hunks)packages/workflow-core/src/lib/index.ts(1 hunks)packages/workflow-core/src/lib/plugins/external-plugin/ballerine-email-plugin.ts(1 hunks)packages/workflow-core/src/lib/plugins/external-plugin/email-plugin.ts(1 hunks)packages/workflow-core/src/lib/workflow-runner.ts(2 hunks)services/workflows-service/prisma/data-migrations(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- services/workflows-service/prisma/data-migrations
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/workflow-core/src/lib/workflow-runner.ts
🔇 Additional comments (6)
packages/workflow-core/src/lib/plugins/external-plugin/ballerine-email-plugin.ts (2)
68-68: LGTM! Good addition for response consistency.
Adding the headers property to the mock response maintains consistency with real HTTP responses, making the interface more reliable for consumers.
Line range hint 77-79: Verify the impact of always returning an empty response object.
The change to always return an empty object ({}) instead of the original responseBody represents a significant behavioral change. Please verify:
- If this aligns with the email plugin's requirements
- If dependent code expects any specific response data
✅ Verification successful
The empty response object is intentional and safe to use
The change to return an empty object is correct because:
- The
BallerineEmailPluginis used solely for sending emails through providers like SendGrid, and the response data is not consumed by any dependent code. - The plugin already handles success/failure through the HTTP response status, not the response body.
- The plugin's parent class
BallerineApiPluginis designed to work with an empty response object, as evidenced by the inheritance pattern. - The email sending functionality is a "fire-and-forget" operation where the actual response content from the email provider is not relevant to the workflow.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for usages of BallerineEmailPlugin to verify response handling
ast-grep --pattern 'new BallerineEmailPlugin($$$)' -A 10
Length of output: 58
Script:
#!/bin/bash
# Search for usages of BallerineEmailPlugin class name in the codebase
rg "BallerineEmailPlugin" -A 5
# Search for any email plugin related configuration or usage
rg "email.*plugin" -i -A 5
# Let's also check the file content to understand the context better
cat "packages/workflow-core/src/lib/plugins/external-plugin/ballerine-email-plugin.ts"
Length of output: 12494
packages/workflow-core/src/index.ts (1)
36-36: LGTM!
The export is properly integrated and maintains consistent ordering with other exports.
packages/workflow-core/src/lib/index.ts (2)
53-54: LGTM!
The exports are well-organized and follow the established pattern of separating type and value exports, consistent with the existing BUILT_IN_EVENT implementation.
Line range hint 1-5: Verify WorkflowRunner integration
The AI summary mentions that this NO_OP action is used in the WorkflowRunner class, but that file isn't included in the review. Let's verify the integration.
✅ Verification successful
Based on the results, I can see that BUILT_IN_ACTION.NO_OP is properly integrated in workflow-runner.ts with both logging (line 470) and action registration (line 478). Let me generate the final response:
Integration with WorkflowRunner verified successfully
The NO_OP action is properly integrated in the WorkflowRunner class:
- Registered as a built-in action handler at
workflow-runner.ts:478 - Includes appropriate logging at
workflow-runner.ts:470
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for usage of NO_OP action in WorkflowRunner
ast-grep --pattern 'BUILT_IN_ACTION.NO_OP'
# Alternatively, search for any reference to ensure proper integration
rg -l "BUILT_IN_ACTION.NO_OP"
Length of output: 10210
packages/workflow-core/src/lib/plugins/external-plugin/email-plugin.ts (1)
Line range hint 69-71: Document the rationale for discarding response body
The change to pass an empty object instead of the original response body might hide useful information. Please:
- Add a comment explaining why the response body is being discarded
- Verify that no critical information is being lost
✅ Verification successful
Based on the analysis of the codebase, I can now provide a final response:
Discarding email response is intentional and safe
The change to pass an empty object instead of the response body in EmailPlugin.returnSuccessResponse() is safe because:
- The base
ApiPlugin.returnSuccessResponse()implementation shows that response data is only used for workflow state transitions viacallbackAction - All email plugin usages in workflow definitions only care about success/failure states (
EMAIL_SENT,EMAIL_FAILURE, etc.) and don't use response data - The email service response typically contains sensitive data like recipient details that shouldn't propagate through the workflow
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any logging or monitoring code that might depend on the email response body
rg -A 5 "returnSuccessResponse.*email" --type ts
Length of output: 50
Script:
#!/bin/bash
# Let's first check the base class implementation to understand the context
ast-grep --pattern 'class $_ extends ExternalPlugin {
$$$
returnSuccessResponse($$$) {
$$$
}
$$$
}'
Length of output: 113
Script:
#!/bin/bash
# Let's also check the email plugin implementation specifically
ast-grep --pattern 'class EmailPlugin extends $_ {
$$$
returnSuccessResponse($$$) {
$$$
}
$$$
}'
Length of output: 6771
Script:
#!/bin/bash
# Let's check for any usage of email plugin response data
rg -A 5 "new EmailPlugin" --type ts
Length of output: 37
Script:
#!/bin/bash
# Let's check the ApiPlugin base class implementation
ast-grep --pattern 'class ApiPlugin {
$$$
returnSuccessResponse($$$) {
$$$
}
$$$
}'
Length of output: 40577
Script:
#!/bin/bash
# Let's check for any test files that might be using email response data
rg -A 5 "EmailPlugin.*test" --type ts
Length of output: 39
Script:
#!/bin/bash
# Let's check for any workflow definitions using email plugin
rg -A 5 "pluginKind.*email" --type ts
Length of output: 12104
Summary by CodeRabbit
New Features
noOpin the WorkflowRunner class.BUILT_IN_ACTIONfor built-in actions.New Versions
@ballerine/workflow-coreand@ballerine/workflow-node-sdkto0.6.76.Bug Fixes
Chores