Store the current flow definition in a state to not depend on a specific workflow version #10352
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Summary
This PR introduces a new Recoil state to manage workflow flow definitions, decoupling workflow visualization from version IDs to better handle draft versions that may change between execution and visualization.
- Added
packages/twenty-front/src/modules/workflow/states/flowState.tsto store workflow definitions directly in state instead of relying on version IDs - Added
packages/twenty-front/src/modules/workflow/hooks/useFlowOrThrow.tsto safely access flow definitions with proper error handling - Added E2E test in
packages/twenty-e2e-testing/tests/workflow-run.spec.tsto verify executed draft versions display correctly after changes - Modified
WorkflowRunVisualizerEffectandWorkflowVersionVisualizerEffectto use flow state instead of version IDs - Refactored step detail components to use flow state directly, removing
RightDrawerWorkflowViewStepContent.tsxin favor of a simpler implementation
14 file(s) reviewed, 11 comment(s)
Edit PR Review Bot Settings | Greptile
| export const useFlowOrThrow = () => { | ||
| const flow = useRecoilValue(flowState); | ||
| if (!isDefined(flow)) { | ||
| throw new Error('Expected the flow to be defined'); |
There was a problem hiding this comment.
style: Consider including more context in the error message about where/when the flow should have been defined
| trigger: WorkflowTrigger | null; | ||
| steps: WorkflowAction[] | null; |
There was a problem hiding this comment.
style: Consider using undefined instead of null for trigger and steps. Using both undefined and null as possible empty values can lead to inconsistent null checks across the codebase.
| } as const; | ||
| } | ||
|
|
||
| if (!isDefined(workflowVersion.steps)) { | ||
| if (!isDefined(steps)) { | ||
| throw new Error( | ||
| 'Malformed workflow version: missing steps information; be sure to create at least one step before trying to edit one', |
There was a problem hiding this comment.
logic: Error message still references 'workflow version' but the function no longer works with version objects
| trigger: workflowRun.output.flow.trigger, | ||
| steps: workflowRun.output.flow.steps, | ||
| }); | ||
| }, [setFlow, workflowRun.output]); |
There was a problem hiding this comment.
style: Consider adding workflowRun as a dependency to handle cases where the entire workflowRun object changes
| setFlow({ | ||
| trigger: currentVersion.trigger, | ||
| steps: currentVersion.steps, | ||
| }); |
There was a problem hiding this comment.
style: Consider deep cloning trigger and steps to prevent potential mutation of currentVersion state
.../twenty-front/src/modules/workflow/workflow-steps/components/RightDrawerWorkflowViewStep.tsx
Outdated
Show resolved
Hide resolved
| } | ||
|
|
||
| const workflowVersion = useWorkflowVersion(workflowVersionId); | ||
| const flow = useFlowOrThrow(); |
There was a problem hiding this comment.
style: useFlowOrThrow could throw before workflowSelectedNode check, causing confusing error messages
| | { | ||
| stepId: string; | ||
| workflowVersion: WorkflowVersion; | ||
| readonly: true; | ||
| onTriggerUpdate?: undefined; | ||
| onActionUpdate?: undefined; | ||
| } | ||
| | { | ||
| stepId: string; |
There was a problem hiding this comment.
logic: Duplicate stepId property in the second union type - this is already defined in the base type on line 25
| stepId: string; | |
| readonly?: false; |
| ...props | ||
| }: WorkflowStepDetailProps) => { | ||
| const stepDefinition = getStepDefinitionOrThrow({ | ||
| stepId, | ||
| workflowVersion, | ||
| trigger, | ||
| steps, | ||
| }); | ||
| if (!isDefined(stepDefinition) || !isDefined(stepDefinition.definition)) { |
There was a problem hiding this comment.
style: Consider throwing an error with more context when stepDefinition or its definition is undefined, rather than silently returning null
| await workflowVisualizer.createInitialTrigger('manual'); | ||
|
|
||
| const manualTriggerAvailabilitySelect = page.getByRole('button', { | ||
| name: 'When record(s) are selected', | ||
| }); | ||
|
|
||
| await manualTriggerAvailabilitySelect.click(); | ||
|
|
||
| const alwaysAvailableOption = page.getByText( | ||
| 'When no record(s) are selected', | ||
| ); | ||
|
|
||
| await alwaysAvailableOption.click(); | ||
|
|
||
| await workflowVisualizer.closeSidePanel(); |
There was a problem hiding this comment.
style: Trigger configuration steps could be extracted into a helper function for reuse in other tests.
.../twenty-front/src/modules/workflow/workflow-steps/components/RightDrawerWorkflowViewStep.tsx
Outdated
Show resolved
Hide resolved
…onents/RightDrawerWorkflowViewStep.tsx Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
|
Thanks @Devessier for your contribution! |
This PR introduces a new Recoil state to store the flow.
A few parts of the application need to know the definition of the current flow. Previously, we stored the workflow version's ID and fetched its definition with the
useWorkflowVersionhook. However, we must use another strategy to visualize workflow runs. Indeed, we now store the definition of the workflow in the workflow run output when it is executed. This is useful for draft versions, which can change between the moment they were executed and the moment they are visualized.