F/sprint24#14
Merged
Merged
Conversation
Prune deprecated contract, discovery, oauth, task-test, and related example assets to reduce maintenance overhead and keep the core domain focused on currently supported flows.
Reviewer's GuideIntroduces multi-draft JSON schema validation support, extends and refactors account-opening/subflow workflows and functions (including new collection/object test utilities and multi-task functions), adds local Mocklab/Dapr docker setup, and removes several legacy task-test, OAuth, contract, and mockoon components. Sequence diagram for multi-task function execution in account-opening domainsequenceDiagram
actor Client
participant Runtime as VNextRuntime
participant Function as MultiTaskFunction
participant MapPolicies as FunctionValidatePoliciesMapping
participant HttpTask1 as HttpTask_validateAccountPolicies
participant MapInstance as FunctionGetInstanceDataMapping
participant GetInstanceTask as GetInstanceDataTask
participant OutputMap as FunctionOutputMapping
participant Mocklab as MocklabService
Client->>Runtime: Start multi-task function
Runtime->>Function: Execute function with tasks
par Validate_account_policies
Function->>MapPolicies: InputHandler(task, context)
MapPolicies->>HttpTask1: SetBody and SetHeaders
MapPolicies-->>Function: ScriptResponse
Function->>HttpTask1: Execute HTTP request
HttpTask1->>Mocklab: POST validate-account-policies
Mocklab-->>HttpTask1: Policy validation response
HttpTask1-->>Function: HTTP result
Function->>MapPolicies: OutputHandler(context)
MapPolicies-->>Function: ScriptResponse policy-check-result
and Get_instance_data
Function->>MapInstance: InputHandler(task, context)
MapInstance->>GetInstanceTask: SetInstance(context.Instance.Key)
MapInstance-->>Function: ScriptResponse
Function->>GetInstanceTask: Execute
GetInstanceTask-->>Function: Instance data snapshot
Function->>MapInstance: OutputHandler(context)
MapInstance-->>Function: ScriptResponse instance-data-result
end
Function->>OutputMap: OutputHandler(context with OutputResponse)
OutputMap-->>Function: ScriptResponse multi-task-function-output
Function-->>Runtime: Aggregated result
Runtime-->>Client: HTTP response with policyValidation and instanceSnapshot
Class diagram for updated account-opening mappings and new multi-task function handlersclassDiagram
class ScriptBase
class IMapping
class IOutputHandler
class WorkflowTask
class HttpTask
class GetInstanceDataTask
class ScriptContext
class ScriptResponse
IMapping <|.. CreateBankAccountMapping
ScriptBase <|-- CreateBankAccountMapping
IMapping <|.. TriggerGetInstanceTaskMapping
IMapping <|.. InitialTransMapping
IMapping <|.. FunctionValidatePoliciesMapping
IMapping <|.. FunctionGetInstanceDataMapping
IMapping <|.. LookupPaymentTypes
IMapping <|.. OnEntryGetInstanceDataMapping
IOutputHandler <|.. FunctionOutputMapping
WorkflowTask <|-- HttpTask
WorkflowTask <|-- GetInstanceDataTask
class CreateBankAccountMapping {
+Task~ScriptResponse~ InputHandler(WorkflowTask task, ScriptContext context)
}
class TriggerGetInstanceTaskMapping {
+Task~ScriptResponse~ InputHandler(WorkflowTask task, ScriptContext context)
}
class InitialTransMapping {
+Task~ScriptResponse~ InputHandler(WorkflowTask task, ScriptContext context)
}
class FunctionValidatePoliciesMapping {
+Task~ScriptResponse~ InputHandler(WorkflowTask task, ScriptContext context)
+Task~ScriptResponse~ OutputHandler(ScriptContext context)
}
class FunctionGetInstanceDataMapping {
+Task~ScriptResponse~ InputHandler(WorkflowTask task, ScriptContext context)
+Task~ScriptResponse~ OutputHandler(ScriptContext context)
}
class LookupPaymentTypes {
+Task~ScriptResponse~ InputHandler(WorkflowTask task, ScriptContext context)
+Task~ScriptResponse~ OutputHandler(ScriptContext context)
}
class OnEntryGetInstanceDataMapping {
+Task~ScriptResponse~ InputHandler(WorkflowTask task, ScriptContext context)
+Task~ScriptResponse~ OutputHandler(ScriptContext context)
}
class FunctionOutputMapping {
+Task~ScriptResponse~ OutputHandler(ScriptContext context)
}
class ScriptContext {
+object Instance
+object Body
+object Headers
+object OutputResponse
}
class ScriptResponse {
+string Key
+object Data
}
Flow diagram for multi-draft JSON schema validation in validate.jsflowchart TD
Start[Start validation for JSON files] --> LoadSchema[Load schema for directory]
LoadSchema --> CheckSchemaField{Schema has $schema field?}
CheckSchemaField -->|Yes| ReadDraft[Read draft from schema.$schema]
CheckSchemaField -->|No| UseDefaultDraft[Use default draft 7]
ReadDraft --> DecideDraft{draft string includes 2019-09?}
UseDefaultDraft --> UseAjv7[Use Ajv7 instance]
DecideDraft -->|Yes| UseAjv2019[Use Ajv2019 instance]
DecideDraft -->|No| UseAjv7
UseAjv7 --> CompileSchema[Compile schema with selected AJV]
UseAjv2019 --> CompileSchema
CompileSchema --> ValidateFiles[Validate JSON files with compiled validator]
ValidateFiles --> End[Report validation results]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Important Review skippedToo many files! This PR contains 189 files, which is 39 over the limit of 150. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (189)
You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request significantly enhances the vNext workflow system by introducing a detailed CLAUDE.md guide, new workflows for testing multi-task functions and dynamic collections, and a master schema for account opening with role-based field visibility. Infrastructure updates include the addition of mocklab for API mocking and improved validation logic in validate.js to support multiple JSON schema drafts. Feedback on the implementation identifies critical compilation errors due to extra semicolons, inconsistent null-conditional access in task responses, and unsafe casting practices. Additionally, corrections are needed for inaccurate documentation comments and minor formatting inconsistencies in the markdown documentation.
| { | ||
| public Task<ScriptResponse> InputHandler(WorkflowTask task, ScriptContext context) | ||
| { | ||
| var httpTask = task as HttpTask;;; |
There was a problem hiding this comment.
This line contains extra semicolons, which will cause a compilation error.
var httpTask = task as HttpTask;
Comment on lines
+14
to
+15
| var policyResult = context.OutputResponse["validateAccountPolicies"].data; | ||
| var instanceData = context.OutputResponse?["getDataFromWorkflow"].data; |
There was a problem hiding this comment.
There's an inconsistent use of the null-conditional operator (?) when accessing context.OutputResponse. On line 14, a direct access is performed, which could lead to a NullReferenceException if OutputResponse is null. However, line 15 correctly uses the null-conditional operator. For safety and consistency, you should use it on both lines.
var policyResult = context.OutputResponse?["validateAccountPolicies"].data;
var instanceData = context.OutputResponse?["getDataFromWorkflow"].data;
| - `core/Views/` — UI component definitions bound to workflow states | ||
| - `core/Schemas/` — JSON Schema definitions for data validation | ||
| - `core/Functions/` — Reusable business logic callable from workflows | ||
| `core/Extensions/` — Runtime capability extensions |
There was a problem hiding this comment.
This list item is missing a leading hyphen. For formatting consistency with the rest of the list, please add one.
Suggested change
| `core/Extensions/` — Runtime capability extensions | |
| - `core/Extensions/` — Runtime capability extensions |
Comment on lines
+5
to
+7
| /// <summary> | ||
| /// Task 2 mapping for multi-task function test. | ||
| /// Fetches current workflow instance data via GetInstanceDataTask. |
There was a problem hiding this comment.
The summary comment for this class appears to be incorrect and likely copied from another file. It refers to a multi-task function test and GetInstanceDataTask, which doesn't match the LookupPaymentTypes class. Please update the comment to accurately describe this class's purpose.
/// <summary>
/// Mapping for the payment-types function.
/// </summary>
| { | ||
| public Task<ScriptResponse> InputHandler(WorkflowTask task, ScriptContext context) | ||
| { | ||
| var triggerTask = (task as GetInstanceDataTask)!; |
There was a problem hiding this comment.
Using the null-forgiving operator (!) here can hide potential runtime errors if the task object is not of type GetInstanceDataTask. A safer approach is to perform a proper type check and handle the case where the cast might fail, for example by throwing an InvalidCastException or InvalidOperationException.
var triggerTask = task as GetInstanceDataTask ?? throw new InvalidOperationException("Task is not a GetInstanceDataTask");
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In
validate.js,getAjvInstanceonly distinguishes draft-2019-09 by a substring check on$schema; consider handling unknown/missing drafts more explicitly (e.g., logging the detected draft or throwing on unsupported versions) to make schema selection and future draft support easier to reason about. - Several new C# mappings declare
async Task<ScriptResponse> OutputHandler(...)but neverawait(e.g.,ParentSharedUpdateMapping,ChildSharedUpdateMapping); either removeasyncand returnTask.FromResult(...)or introduce awaited async work to avoid unnecessary state-machine overhead and compiler warnings. - In
ParentInitialGetInstanceDataMapping,triggerTask.SetInstance("50044086189")is hard-coded; consider deriving the instance identifier from configuration orScriptContextto avoid baking environment-specific IDs into the workflow logic.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `validate.js`, `getAjvInstance` only distinguishes draft-2019-09 by a substring check on `$schema`; consider handling unknown/missing drafts more explicitly (e.g., logging the detected draft or throwing on unsupported versions) to make schema selection and future draft support easier to reason about.
- Several new C# mappings declare `async Task<ScriptResponse> OutputHandler(...)` but never `await` (e.g., `ParentSharedUpdateMapping`, `ChildSharedUpdateMapping`); either remove `async` and return `Task.FromResult(...)` or introduce awaited async work to avoid unnecessary state-machine overhead and compiler warnings.
- In `ParentInitialGetInstanceDataMapping`, `triggerTask.SetInstance("50044086189")` is hard-coded; consider deriving the instance identifier from configuration or `ScriptContext` to avoid baking environment-specific IDs into the workflow logic.
## Individual Comments
### Comment 1
<location path="docker-compose.yml" line_range="18-22" />
<code_context>
+ networks:
+ - bbt-development
+
+ mocklab-dapr:
+ image: daprio/daprd:latest
+ container_name: mocklab-dapr
+ command:
+ - ./daprd
+ - --app-id
+ - mocklab
</code_context>
<issue_to_address>
**issue (bug_risk):** Revisit daprd invocation; using './daprd' with the daprd image may not be necessary and could fail.
The `daprio/daprd` image already uses `daprd` as its entrypoint, so the command should usually be just the flags (e.g. `[
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+18
to
+22
| mocklab-dapr: | ||
| image: daprio/daprd:latest | ||
| container_name: mocklab-dapr | ||
| command: | ||
| - ./daprd |
There was a problem hiding this comment.
issue (bug_risk): Revisit daprd invocation; using './daprd' with the daprd image may not be necessary and could fail.
The daprio/daprd image already uses daprd as its entrypoint, so the command should usually be just the flags (e.g. `[
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.
Summary by Sourcery
Add new collection/object workflow tests, multi-task account-opening functions, and local mock infrastructure while updating validation and simplifying existing workflows.
New Features:
Bug Fixes:
Enhancements:
Build:
Documentation:
Tests:
Chores: