-
Notifications
You must be signed in to change notification settings - Fork 4.1k
Allow to insert into loop step ids #14425
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Greptile Summary
This PR introduces comprehensive support for inserting workflow steps into iterator loop structures. The main changes include:
Backend Changes:
- Adds a new
WorkflowStepCreationOptions
type with iterator-specific settings (iteratorStepId
,shouldInsertToLoop
) - Introduces an optional
options
field to theCreateWorkflowVersionStepInput
DTO for controlling step insertion behavior - Refactors the
insertStep
utility to handle loop insertion via newupdateStepsWithOptions
function that managesinitialLoopStepIds
arrays - Updates
removeStep
utility to properly clean up iterator references when steps are deleted from loops - Adds automatic empty node creation when iterator steps are created through the new
createEmptyNodeForIteratorStep
method - Simplifies type handling by removing legacy string/array union types for
initialLoopStepIds
, standardizing tostring[]
only
Frontend Changes:
- Introduces a new
EMPTY
workflow action type with corresponding schema and UI components - Adds
WorkflowEditActionEmpty
component as a placeholder/skeleton for empty workflow steps - Updates workflow rendering components to handle the new EMPTY action type with appropriate styling (tertiary font color)
- Removes manual UI input for
initialLoopStepIds
in the iterator component, moving to automated management
Schema Changes:
- Adds
workflowEmptyActionSchema
andworkflowEmptyActionSettingsSchema
to support placeholder nodes - Updates the workflow action discriminated union to include the new EMPTY type
- Standardizes
initialLoopStepIds
typing by removing the temporary string union workaround
The implementation follows a pattern where iterator creation automatically generates empty placeholder nodes that provide visual insertion points for users to add steps within loop structures. This replaces the previous manual step ID management approach with a more intuitive drag-and-drop style workflow builder experience.
Confidence score: 3/5
- This PR introduces significant complexity around iterator loop management that could cause issues if edge cases aren't properly handled
- Score reflects the extensive changes to core workflow logic including step creation, removal, and iterator handling that require careful testing
- Pay close attention to the iterator step creation logic in
workflow-version-step.workspace-service.ts
and the loop insertion handling ininsert-step.ts
21 files reviewed, 2 comments
...enty-server/src/modules/workflow/workflow-builder/workflow-version-step/utils/insert-step.ts
Outdated
Show resolved
Hide resolved
const iteratorStep: WorkflowIteratorAction = { | ||
id: '1', | ||
name: 'Iterator 1', | ||
type: WorkflowActionType.ITERATOR, | ||
settings: { | ||
input: { | ||
initialLoopStepIds: ['existing-loop-step'], | ||
items: [], | ||
}, | ||
outputSchema: {}, | ||
errorHandlingOptions: { | ||
retryOnFailure: { value: false }, | ||
continueOnFailure: { value: false }, | ||
}, | ||
}, | ||
valid: true, | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: Consider extracting this iterator step mock to a helper function since it's duplicated in both test cases
const iteratorStep: WorkflowIteratorAction = { | |
id: '1', | |
name: 'Iterator 1', | |
type: WorkflowActionType.ITERATOR, | |
settings: { | |
input: { | |
initialLoopStepIds: ['existing-loop-step'], | |
items: [], | |
}, | |
outputSchema: {}, | |
errorHandlingOptions: { | |
retryOnFailure: { value: false }, | |
continueOnFailure: { value: false }, | |
}, | |
}, | |
valid: true, | |
}; | |
const createMockIteratorAction = ( | |
id: string, | |
initialLoopStepIds: string[] = [], | |
): WorkflowIteratorAction => ({ | |
id, | |
name: `Iterator ${id}`, | |
type: WorkflowActionType.ITERATOR, | |
settings: { | |
input: { | |
initialLoopStepIds, | |
items: [], | |
}, | |
outputSchema: {}, | |
errorHandlingOptions: { | |
retryOnFailure: { value: false }, | |
continueOnFailure: { value: false }, | |
}, | |
}, | |
valid: true, | |
}); |
446ff66
to
3b517ef
Compare
🚀 Preview Environment Ready! Your preview environment is available at: http://bore.pub:31607 This environment will automatically shut down when the PR is closed or after 5 hours. |
@@ -722,6 +733,12 @@ export class WorkflowVersionStepWorkspaceService { | |||
}; | |||
} | |||
case WorkflowActionType.ITERATOR: { | |||
const emptyNodeStep = await this.createEmptyNodeForIteratorStep({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As we do that we should update the front in order to avoid adding a fake loop step. Maybe in another PR if you prefer
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
leaving it to @Devessier 😏
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great job! As we discussed, I suggest a small renaming.
330525e
to
30d25ef
Compare
Enregistrement.de.l.ecran.2025-09-11.a.16.56.29.mov