-
Notifications
You must be signed in to change notification settings - Fork 3.9k
Workflow runs in side panel #11669
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
Workflow runs in side panel #11669
Conversation
…croll on top of the canvas
…lds in the side panel
reactflow calls selection change handlers even when a node is set as selected at the first render of the Reactflow component. We don't want to run the callback at this moment.
3495b8c
to
e8c45a6
Compare
… record from the side panel
…run in side panel
…ys reset between two side panel views
set( | ||
activeTabIdComponentState.atomFamily({ | ||
instanceId: WORKFLOW_RUN_STEP_SIDE_PANEL_TAB_LIST_COMPONENT_ID, | ||
}), | ||
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.
No longer needed as we now the page's id as the tab list id.
const activeTabId = useRecoilComponentValueV2( | ||
activeTabIdComponentState, | ||
WORKFLOW_RUN_STEP_SIDE_PANEL_TAB_LIST_COMPONENT_ID, | ||
commandMenuPageComponentInstance.instanceId, |
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.
Thanks to this, the tab is scoped to the side panel's page. The tab will automatically reset when opening a new node in the side panel.
@@ -145,7 +145,7 @@ export const useRecordShowContainerTabs = ( | |||
tabs: { | |||
workflow: { | |||
title: 'Flow', | |||
position: 0, | |||
position: 101, |
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.
Do as with the other object specific layouts: position it after the fields in the side panel.
const { success, data: record } = useMemo( | ||
() => workflowRunSchema.safeParse(rawRecord), | ||
[rawRecord], | ||
); |
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.
Memoize to prevent useEffect unwanted re-runs.
…ial tab in openWorkflowRunViewStepInCommandMenu
…opened in side panel then full screen
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 PR implements workflow run visualization in the side panel, with support for form steps and improved diagram rendering management.
- Added
/packages/twenty-front/src/modules/workflow/workflow-diagram/states/workflowDiagramStatusState.ts
to manage diagram rendering phases ('computing-diagram', 'computing-dimensions', 'done') - Added e2e tests in
/packages/twenty-e2e-testing/tests/workflow-run.spec.ts
for workflow run visualization and form step handling - Added
useSetInitialWorkflowRunRightDrawerTab
hook to manage initial tab selection based on execution status - Optimized workflow run validation with
useMemo
inuseWorkflowRun.ts
- Improved viewport management with right drawer in
WorkflowDiagramCanvasBase.tsx
19 file(s) reviewed, 7 comment(s)
Edit PR Review Bot Settings | Greptile
// 1. Exit the dropdown | ||
await workflowRunNameCell.click(); | ||
// 2. Actually open the workflow run in the side panel | ||
await workflowRunNameCell.click(); |
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: Double click on workflowRunNameCell could be flaky. Consider using a more reliable way to exit dropdown and open side panel, or add wait conditions between clicks.
if ( | ||
!isDefined(workflowRun) || | ||
workflowDiagramStatus === 'computing-diagram' | ||
) { | ||
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 showing a loading spinner or skeleton instead of returning null during computation
if (!isDefined(containerRef.current)) { | ||
return; | ||
} |
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: Early return without calling onInit() could lead to missed initialization. Consider calling onInit() even if containerRef is not defined
const setInitialWorkflowRunRightDrawerTab = useRecoilCallback( | ||
({ snapshot, set }) => | ||
({ | ||
workflowSelectedNode, | ||
stepExecutionStatus, | ||
}: { | ||
workflowSelectedNode: string; | ||
stepExecutionStatus: WorkflowDiagramRunStatus; | ||
}) => { | ||
const commandMenuPageInfo = getSnapshotValue( | ||
snapshot, | ||
commandMenuPageInfoState, | ||
); | ||
|
||
const activeWorkflowRunRightDrawerTab = getSnapshotValue( | ||
snapshot, | ||
activeTabIdComponentState.atomFamily({ | ||
instanceId: commandMenuPageInfo.instanceId, | ||
}), | ||
) as WorkflowRunTabId | null; | ||
|
||
const isInputTabDisabled = getIsInputTabDisabled({ | ||
stepExecutionStatus, | ||
workflowSelectedNode, | ||
}); | ||
const isOutputTabDisabled = getIsOutputTabDisabled({ | ||
stepExecutionStatus, | ||
}); | ||
|
||
if (isNull(activeWorkflowRunRightDrawerTab)) { | ||
const defaultTabId = isOutputTabDisabled | ||
? WorkflowRunTabId.NODE | ||
: WorkflowRunTabId.OUTPUT; | ||
|
||
set( | ||
activeTabIdComponentState.atomFamily({ | ||
instanceId: commandMenuPageInfo.instanceId, | ||
}), | ||
defaultTabId, | ||
); | ||
|
||
return; | ||
} | ||
|
||
if ( | ||
(isInputTabDisabled && | ||
activeWorkflowRunRightDrawerTab === WorkflowRunTabId.INPUT) || | ||
(isOutputTabDisabled && | ||
activeWorkflowRunRightDrawerTab === WorkflowRunTabId.OUTPUT) | ||
) { | ||
set( | ||
activeTabIdComponentState.atomFamily({ | ||
instanceId: commandMenuPageInfo.instanceId, | ||
}), | ||
WorkflowRunTabId.NODE, | ||
); | ||
} | ||
}, | ||
[], | ||
); |
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: Dependencies array is empty but the callback uses external functions getIsInputTabDisabled and getIsOutputTabDisabled. Consider adding these as dependencies
expect(result.nodes[0].selected).toBe(true); | ||
expect(result.nodes[1].selected).toBe(false); |
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: Test should verify that all other nodes are deselected, not just node[1].
expect(result.nodes[0].selected).toBe(true); | |
expect(result.nodes[1].selected).toBe(false); | |
expect(result.nodes[0].selected).toBe(true); | |
expect(result.nodes.slice(1).every(node => !node.selected)).toBe(true); |
} | ||
| undefined | ||
>({ | ||
key: 'workflowStepIdToOpenByDefaultState', |
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: Key name 'workflowStepIdToOpenByDefaultState' doesn't match variable name 'workflowRunStepToOpenByDefaultState'. Consider using consistent naming.
if (node.id === nodeIdToSelect) { | ||
return { | ||
...node, | ||
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.
logic: This only sets selected=true but never sets selected=false for other nodes. Could lead to multiple selected nodes.
if (node.id === nodeIdToSelect) { | |
return { | |
...node, | |
selected: true, | |
}; | |
} | |
if (node.id === nodeIdToSelect) { | |
return { | |
...node, | |
selected: true, | |
}; | |
} | |
return { | |
...node, | |
selected: false, | |
}; |
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 work 💪 small comments but LGTM!
Vidéo explicative : https://share.cleanshot.com/VsvWknlW
Closes twentyhq/core-team-issues#810
Closes twentyhq/core-team-issues#806
Known issues to fix later: