Skip to content
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

Merged
merged 15 commits into from
Oct 8, 2024
Merged

Conversation

Devessier
Copy link
Contributor

@Devessier Devessier commented Oct 7, 2024

In this PR:

  • Refactored components to clarify their behavior. For example, I renamed the Workflow component to WorkflowVisualizer. This moved forward the issue twentyhq/private-issues#120.
  • 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)

CleanShot.2024-10-07.at.17.33.36.mp4

@Devessier Devessier force-pushed the create-workflow-version-show-page branch from 47ba3eb to 2a71441 Compare October 7, 2024 12:21
@Devessier Devessier marked this pull request as ready for review October 7, 2024 16:51
Copy link
Contributor

@greptile-apps greptile-apps bot left a 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,
Copy link
Contributor

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,
Copy link
Contributor

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

Comment on lines +15 to +17
if (!isDefined(workflowVersion)) {
return null;
}
Copy link
Contributor

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

Comment on lines +14 to +16
throw new Error(
'Expected a node to be selected. Selecting a node is mandatory to edit it.',
);
Copy link
Contributor

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.

Comment on lines 54 to 56
reactflow.updateNode(workflowDiagramTriggerNodeSelection, {
selected: true,
});
Copy link
Contributor

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

}
}
}
}
Copy link
Contributor

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

Comment on lines +46 to +48
// eslint-disable-next-line react/jsx-props-no-spreading
{...props}
Copy link
Contributor

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

Comment on lines +23 to +33
useEffect(() => {
if (!isDefined(workflowVersion)) {
setWorkflowDiagram(undefined);

return;
}

const nextWorkflowDiagram = getWorkflowVersionDiagram(workflowVersion);

setWorkflowDiagram(nextWorkflowDiagram);
}, [setWorkflowDiagram, workflowVersion]);
Copy link
Contributor

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;
Copy link
Contributor

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

Comment on lines +26 to +31
{isDefined(workflowDiagram) && isDefined(workflowWithCurrentVersion) ? (
<WorkflowDiagramCanvasEditable
diagram={workflowDiagram}
workflowWithCurrentVersion={workflowWithCurrentVersion}
/>
) : null}
Copy link
Contributor

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',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not workflow version?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks a bit strange but we'll go with that for now. I think it's better than "Flow".

CleanShot 2024-10-08 at 17 08 21@2x

const {
records: draftWorkflowVersions,
loading: loadingDraftWorkflowVersions,
} = useFindManyRecords<WorkflowVersion>({
Copy link
Contributor

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

Copy link
Contributor Author

@Devessier Devessier Oct 8, 2024

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:

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh... Too bad

@Devessier Devessier force-pushed the create-workflow-version-show-page branch from 78eb82d to b712d69 Compare October 8, 2024 15:15
@Devessier Devessier merged commit 1863636 into main Oct 8, 2024
13 checks passed
@Devessier Devessier deleted the create-workflow-version-show-page branch October 8, 2024 16:16
Copy link

github-actions bot commented Oct 8, 2024

Thanks @Devessier for your contribution!
This marks your 12th PR on the repo. You're top 3% of all our contributors 🎉
See contributor page - Share on LinkedIn - Share on Twitter

Contributions

harshit078 pushed a commit to harshit078/twenty that referenced this pull request Oct 14, 2024
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants