-
Notifications
You must be signed in to change notification settings - Fork 3.9k
Fix workflow run ouput format #10302
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
Conversation
5cf0cd0
to
37db4dc
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 PR refactors the workflow run output format across the frontend and backend to improve workflow execution tracking and visualization.
- Changed
WorkflowRunOutput
type in/packages/twenty-front/src/modules/workflow/types/Workflow.ts
to include aflow
object withtrigger
andsteps
, makingstepsOutput
optional - Simplified
WorkflowRunVisualizerEffect
in/packages/twenty-front/src/modules/workflow/workflow-diagram/components/WorkflowRunVisualizerEffect.tsx
to use workflow run output directly - Added optional chaining for
stepsOutput
access in/packages/twenty-server/src/modules/workflow/workflow-executor/workspace-services/workflow-executor.workspace-service.ts
to prevent runtime errors - Re-enabled 'Flow' tab for workflow run views in
/packages/twenty-front/src/modules/object-record/record-show/hooks/useRecordShowContainerTabs.ts
- Updated test cases in
/packages/twenty-front/src/modules/workflow/workflow-diagram/utils/__tests__/generateWorkflowRunDiagram.test.ts
to reflect new output format
8 file(s) reviewed, 2 comment(s)
Edit PR Review Bot Settings | Greptile
title: 'Flow', | ||
position: 0, | ||
Icon: IconSettings, |
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: Both workflowRunOutput and workflowRunFlow tabs have position 0, which could cause unpredictable tab ordering
attemptCount: 1, | ||
result: undefined, | ||
error: '', | ||
}, |
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: Empty error string treated as failure state. Consider using undefined for consistency with other error cases.
@@ -231,7 +231,7 @@ export class WorkflowVisualizerPage { | |||
|
|||
async closeSidePanel() { | |||
const closeButton = this.#page.getByTestId( | |||
'page-header-close-command-menu-button', | |||
'page-header-command-menu-button', |
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 this renaming?
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.
My change concerns the e2e tests, and I'm not responsible for the renaming. However, I think it's due to Raphaël's recent changes on the Cmd+K close button. I assume the Open and Closed states are now the same button, hence a more global test id. (close-button -> button)
Thanks @Devessier for your contribution! |
Suspect IssuesThis pull request was deployed and Sentry observed the following issues:
Did you find this useful? React with a 👍 or 👎 |
Uh oh!
There was an error while loading. Please reload this page.