Build separated executors #10370

thomtrp · 2025-02-20T17:44:03Z

We now have two executors. WorkflowExecutor is responsible for:

find the right executor for that step
call that executor
save the output
handle faillure management
call itself on next step if there is one

WorkflowActionExecutor finds the right action to execute.

Both returns a base output so the parent caller can also build its output.
I also duplicated the execution functions so we keep on handling the old step format.

Next steps:

Front should be able to read both V1 and V2 format
New steps should be using V2 format. Updates should depend on step current format
Run a script to migrate V1 to V2
Remove V1 handler

greptile-apps

PR Summary

This PR introduces a significant architectural change to the workflow execution system by separating concerns into distinct WorkflowExecutor and WorkflowActionExecutor components.

Added new WorkflowStep type in /packages/twenty-server/src/modules/workflow/workflow-executor/types/workflow-step.type.ts to support V2 workflow system
Introduced WorkflowStepResult in /packages/twenty-server/src/modules/workflow/workflow-executor/types/workflow-step-result.type.ts replacing WorkflowActionResult for standardized output handling
Added type guards isWorkflowStep and isWorkflowAction in /packages/twenty-server/src/modules/workflow/workflow-executor/guards/ for step validation
Implemented dual execution paths in workflow-action-executor.workspace-service.ts to maintain backward compatibility while introducing new step format
Restructured modules to better separate concerns, moving action-specific logic into WorkflowActionExecutorModule

_{25 file(s) reviewed, 13 comment(s)}
_{Edit PR Review Bot Settings | Greptile}

packages/twenty-server/src/modules/workflow/workflow-executor/guards/is-workflow-step.guard.ts

.../twenty-server/src/modules/workflow/common/standard-objects/workflow-run.workspace-entity.ts

...ages/twenty-server/src/modules/workflow/workflow-executor/types/workflow-step-result.type.ts

packages/twenty-server/src/modules/workflow/workflow-executor/types/workflow-step.type.ts

...ules/workflow/workflow-executor/workflow-actions/record-crud/find-records.workflow-action.ts

packages/twenty-server/src/modules/workflow/workflow-runner/jobs/run-workflow.job.ts

...modules/workflow/workflow-executor/workspace-services/workflow-executor.workspace-service.ts

Devessier

I'm really excited by these changes! I wrote a few comments, let's discuss about that. I'm already approving the PR as there is nothing critical.

Great job on the retrocompatibility! We can still execute workflows properly.

Devessier · 2025-02-21T14:44:56Z

...ages/twenty-server/src/modules/workflow/workflow-executor/guards/is-workflow-action.guard.ts

+export const isWorkflowAction = (
+  step: WorkflowStep | WorkflowAction,
+): step is WorkflowAction => {
+  return 'settings' in step;
+};


I love these guards 👌

Maybe we can check the value of the type property. It seems to be the most substantial separation between the two types.

Suggested change

export const isWorkflowAction = (

step: WorkflowStep | WorkflowAction,

): step is WorkflowAction => {

return 'settings' in step;

};

export const isWorkflowAction = (

step: WorkflowStep | WorkflowAction,

): step is WorkflowAction => {

return Object.values(WorkflowActionType).includes(

step.type as WorkflowActionType,

);

};

I'm unsure how we usually do that in the back-end codebase.

Devessier · 2025-02-21T14:52:51Z

packages/twenty-server/src/modules/workflow/workflow-executor/guards/is-workflow-step.guard.ts

+export const isWorkflowStep = (
+  step: WorkflowStep | WorkflowAction,
+): step is WorkflowStep => {
+  return 'stepSettings' in step;
+};


Same thing here. I would check the type property as it's the most significant difference between the two types.

Suggested change

export const isWorkflowStep = (

step: WorkflowStep | WorkflowAction,

): step is WorkflowStep => {

return 'stepSettings' in step;

};

export const isWorkflowStep = (

step: WorkflowStep | WorkflowAction,

): step is WorkflowStep => {

return Object.values(WorkflowStepType).includes(

step.type as WorkflowStepType,

);

};

Devessier · 2025-02-21T14:56:15Z

packages/twenty-server/src/modules/workflow/workflow-executor/types/workflow-step.type.ts

+/**
+ * To migrate to WorkflowActionStepSettings once the V2 workflow is released.
+ */


Devessier · 2025-02-21T15:19:39Z

...modules/workflow/workflow-executor/workspace-services/workflow-executor.workspace-service.ts

-    const updatedStepsOutput = {
-      ...workflowExecutorState.stepsOutput,
-      [step.id]: updatedStepOutput,
+    if (result.result) {


🤓 nitpick: ‏result.result. Maybe we can rename the result variable to read stepExecution.result

Devessier · 2025-02-21T15:20:20Z

...modules/workflow/workflow-executor/workspace-services/workflow-executor.workspace-service.ts

+      return this.execute({
+        workflowRunId,
+        currentStepIndex: currentStepIndex + 1,
+        steps,
+        context,
+      });
+    }


Excellent 👍

Devessier · 2025-02-21T15:42:34Z

...modules/workflow/workflow-executor/workspace-services/workflow-executor.workspace-service.ts


-    let result: WorkflowActionResult;
+    const stepExecutor = this.getStepExecutor(step);
+    let result;


The result variable was typed in the previous version. Why did you remove the explicit type?

Devessier · 2025-02-21T15:42:57Z

...modules/workflow/workflow-executor/workspace-services/workflow-executor.workspace-service.ts

+      );
+    }
+
+    let result;


I think it would be great to strictly type the result variable.

Devessier · 2025-02-21T15:43:46Z

...enty-server/src/modules/workflow/workflow-executor/interfaces/workflow-executor.interface.ts

+import { WorkflowExecutorInput } from 'src/modules/workflow/workflow-executor/types/workflow-executor-input';
+import { WorkflowStepResult } from 'src/modules/workflow/workflow-executor/types/workflow-step-result.type';
+
+export interface WorkflowExecutor {


I like this interface 💪

Devessier · 2025-02-21T15:45:26Z

...modules/workflow/workflow-executor/workspace-services/workflow-executor.workspace-service.ts

+    } else {
+      return this.executeStep({
+        currentStepIndex,
+        steps,
+        context,
+        attemptCount,
+        workflowRunId,
+      });
+    }


🤓 nitpick: I think it would be "safer" to use else if (isWorkflowStep(step)) to not fall in an unwanted case.‏

Devessier · 2025-02-21T15:48:36Z

packages/twenty-server/src/engine/core-modules/workflow/dtos/workflow-step.dto.ts

+  settings?: object;
+
+  @Field(() => graphqlTypeJson)
+  stepSettings?: object;


Could you please remind me why we split the settings into two properties?

settings is the previous field that will be copied into actionSettings, subfield of stepSettings

greptile-apps bot reviewed Feb 20, 2025

View reviewed changes

thomtrp added 3 commits February 21, 2025 14:58

Build separated executors

921ef6e

Greptile feedbacks

b78b64e

Fix serverless function publication

3b9c156

thomtrp force-pushed the add-new-workflow-executor branch from 9fabd21 to 3b9c156 Compare February 21, 2025 13:59

Remove unused checks

9ee4d49

thomtrp force-pushed the add-new-workflow-executor branch from ed7a80f to 9ee4d49 Compare February 21, 2025 14:19

Devessier self-requested a review February 21, 2025 14:22

Devessier approved these changes Feb 21, 2025

View reviewed changes

thomtrp closed this Feb 21, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Build separated executors #10370

Build separated executors #10370

thomtrp commented Feb 20, 2025 •

edited

Loading

greptile-apps bot left a comment

Devessier left a comment

Devessier Feb 21, 2025

Devessier Feb 21, 2025

Devessier Feb 21, 2025

Devessier Feb 21, 2025

Devessier Feb 21, 2025

Devessier Feb 21, 2025

Devessier Feb 21, 2025

Devessier Feb 21, 2025

Devessier Feb 21, 2025

Devessier Feb 21, 2025

Devessier Feb 21, 2025

thomtrp Feb 21, 2025

Build separated executors #10370

Build separated executors #10370

Conversation

thomtrp commented Feb 20, 2025 • edited Loading

greptile-apps bot left a comment

Choose a reason for hiding this comment

PR Summary

Devessier left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thomtrp commented Feb 20, 2025 •

edited

Loading