-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Create workflow version show page #7466
Conversation
47ba3eb
to
2a71441
Compare
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.
PR Summary
This pull request implements a read-only view for workflow versions, enhancing the workflow management functionality in the Twenty application.
- Added new components: WorkflowVersionVisualizer and WorkflowDiagramCanvasReadonly for displaying workflows in read-only mode
- Implemented RecordShowPageWorkflowVersionHeader with buttons to activate, deactivate, or use a workflow version as a draft
- Created RightDrawerWorkflowViewStep and RightDrawerWorkflowViewStepContent for viewing workflow steps in the right drawer
- Refactored existing components to support both editable and read-only modes, improving code reusability
- Updated ShowPageRightContainer to include tabs for workflow and workflow version visualization
30 file(s) reviewed, 14 comment(s)
Edit PR Review Bot Settings
Pick<Workflow, '__typename' | 'id' | 'lastPublishedVersionId'> | ||
>({ | ||
objectNameSingular: CoreObjectNameSingular.Workflow, | ||
objectRecordId: workflowVersion?.workflowId, |
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 adding a null check for workflowVersion before accessing workflowId
objectNameSingular: CoreObjectNameSingular.WorkflowVersion, | ||
filter: { | ||
workflowId: { | ||
eq: workflowVersion?.workflow.id, |
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: Ensure workflowVersion.workflow is defined before accessing id
if (!isDefined(workflowVersion)) { | ||
return null; | ||
} |
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: Add a loading state or error message when workflowVersion is undefined
throw new Error( | ||
'Expected a node to be selected. Selecting a node is mandatory to edit it.', | ||
); |
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.
logic: The error message mentions editing, but this component is for viewing. Consider updating the message to reflect the view-only nature.
reactflow.updateNode(workflowDiagramTriggerNodeSelection, { | ||
selected: 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: Wrap this update in a try-catch block to handle potential errors if the node doesn't exist
} | ||
} | ||
} | ||
} |
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.
logic: Add a default case to handle unknown step types
// eslint-disable-next-line react/jsx-props-no-spreading | ||
{...props} |
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 using a custom hook or utility function to spread props safely instead of disabling the eslint rule
useEffect(() => { | ||
if (!isDefined(workflowVersion)) { | ||
setWorkflowDiagram(undefined); | ||
|
||
return; | ||
} | ||
|
||
const nextWorkflowDiagram = getWorkflowVersionDiagram(workflowVersion); | ||
|
||
setWorkflowDiagram(nextWorkflowDiagram); | ||
}, [setWorkflowDiagram, workflowVersion]); |
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: This effect might run unnecessarily if setWorkflowDiagram changes. Consider using useCallback for setWorkflowDiagram or moving it inside the effect
setWorkflowDiagram(nextWorkflowDiagram); | ||
}, [setWorkflowDiagram, workflowVersion]); | ||
|
||
return null; |
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 using React.Fragment instead of returning null for consistency
{isDefined(workflowDiagram) && isDefined(workflowWithCurrentVersion) ? ( | ||
<WorkflowDiagramCanvasEditable | ||
diagram={workflowDiagram} | ||
workflowWithCurrentVersion={workflowWithCurrentVersion} | ||
/> | ||
) : null} |
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 conditional rendering into a separate component for better readability
@@ -212,6 +221,12 @@ export const ShowPageRightContainer = ({ | |||
Icon: IconSettings, | |||
hide: !isWorkflow, | |||
}, | |||
{ | |||
id: 'flow', | |||
title: 'Flow', |
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.
why not workflow version?
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.
const { | ||
records: draftWorkflowVersions, | ||
loading: loadingDraftWorkflowVersions, | ||
} = useFindManyRecords<WorkflowVersion>({ |
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.
could you add a comment there? This part should be removed once statuses are fixed on workflow. That will save us a query
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.
I will do. However, we'll have to keep that code as we need to know what is the id of the current draft version – if any – to make the "Use as Draft" button work.
See:
twenty/packages/twenty-front/src/modules/workflow/components/RecordShowPageWorkflowVersionHeader.tsx
Line 83 in 3e75a60
idToUpdate: draftWorkflowVersions[0].id, |
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.
Oh... Too bad
…and display a readonly visualizer for workflow versions
…the status of the version
…ly published workflow version
…d to trigger node selection after actually selecting a node
78eb82d
to
b712d69
Compare
Thanks @Devessier for your contribution! |
In this PR: - Refactored components to clarify their behavior. For example, I renamed the `Workflow` component to `WorkflowVisualizer`. This moved forward the issue #7010. - Create two variants of several workflow-related components: one version for editing and another for viewing. For instance, there is `WorkflowDiagramCanvasEditable.tsx` and `WorkflowDiagramCanvasReadonly.tsx` - Implement the show page for workflow versions. On this page, we display a readonly workflow visualizer. Users can click on nodes and it will expand the right drawer. - I added buttons in the header of the RecordShowPage for workflow versions: users can activate, deactivate or use the currently viewed version as the next draft. **There are many cache desynchronisation and I'll fix them really soon.** ## Demo (Turn sound on) https://github.com/user-attachments/assets/97fafa48-8902-4dab-8b39-f40848bf041e
In this PR:
Workflow
component toWorkflowVisualizer
. This moved forward the issue twentyhq/private-issues#120.WorkflowDiagramCanvasEditable.tsx
andWorkflowDiagramCanvasReadonly.tsx
There are many cache desynchronisation and I'll fix them really soon.
Demo
(Turn sound on)
CleanShot.2024-10-07.at.17.33.36.mp4