-
Notifications
You must be signed in to change notification settings - Fork 4.9k
If else node followup changes #16974
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
This update simplifies the useDeleteStep hook by eliminating the logic related to handling IF_ELSE steps and their associated child steps, which were previously checked and deleted if empty. The code now focuses solely on deleting the specified workflow version step.
…ions This update introduces logic to identify and remove empty child step IDs when an IF_ELSE step is deleted. The function now computes all removed step IDs, including those from child steps, ensuring a more comprehensive cleanup of the workflow state. Additionally, the code has been refactored for clarity and efficiency.
This update modifies the useDeleteWorkflowVersionStep hook to incorporate a refetching mechanism for the workflow version query after a deletion. It adds a new dependency on the useFindOneRecordQuery hook to ensure the latest data is retrieved, improving the accuracy of the workflow state post-deletion.
…onents This update simplifies the WorkflowStepFilterColumn by removing unused props and logic related to child step filters, enhancing readability. In the WorkflowIfElseBranchEditor, a new function isLastFilterInIfBranch is introduced to determine the last filter in an IF branch, improving the handling of filter visibility based on branch context. Overall, these changes streamline the workflow filter components for better maintainability.
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.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
|
🚀 Preview Environment Ready! Your preview environment is available at: http://bore.pub:39683 This environment will automatically shut down when the PR is closed or after 5 hours. |
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.
1 issue found across 5 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="packages/twenty-server/src/modules/workflow/workflow-builder/workflow-version-step/utils/remove-step.ts">
<violation number="1" location="packages/twenty-server/src/modules/workflow/workflow-builder/workflow-version-step/utils/remove-step.ts:108">
P2: If-else branch nextStepIds only filter out removed step IDs but don't use `computeUpdatedNextStepIds` to reconnect children. This inconsistency with how regular steps, iterators, and triggers handle deletion may break workflow chains when a branch references a deleted step whose children should be preserved.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
...enty-server/src/modules/workflow/workflow-builder/workflow-version-step/utils/remove-step.ts
Outdated
Show resolved
Hide resolved
This update simplifies the logic in the removeStep utility by directly constructing the array of removed step IDs, eliminating the unnecessary inclusion of child step IDs when they are not provided. This change enhances code clarity and maintains the functionality of removing empty child steps.
This update enhances the removeStep utility by refining the logic for updating nextStepIds in branches. It now computes the updated nextStepIds directly within the mapping function, ensuring that the removal of step IDs is handled more efficiently and clearly. This change maintains the functionality of the utility while improving code readability.
| if (branch.nextStepIds.includes(stepIdToDelete)) { | ||
| updatedNextStepIds = computeUpdatedNextStepIds({ | ||
| existingNextStepIds: branch.nextStepIds, | ||
| stepIdToRemove: stepIdToDelete, | ||
| stepToDeleteChildrenIds: allStepToDeleteChildrenIds, | ||
| }); | ||
| } |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This update streamlines the logic in the removeStep utility by removing the unnecessary inclusion of empty child step IDs when constructing the array of step IDs to delete. This change enhances code clarity while maintaining the existing functionality of the utility.
… step IDs This update simplifies the removeStep utility by removing the unnecessary variable that stored child step IDs, directly passing the parameter instead. This change enhances code clarity and maintains the existing functionality of the utility.
| const result = await mutate({ | ||
| variables: { input }, | ||
| awaitRefetchQueries: true, | ||
| refetchQueries: [ |
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.
if we stop removing the empty node, we can remove that refetch logic. Updating cache will be enough
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.
@FelixMalfait suggested that we should remove the empty nodes. LMK what should we do @thomtrp ?
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.
let's keep it for now
| .map((step) => { | ||
| if ( | ||
| step.type === WorkflowActionType.IF_ELSE && | ||
| isWorkflowIfElseAction(step) |
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.
This behavior looks huge and is not consistent with iterators. For iterators, we let the user remove the empty node. The user can delete it by himself. I think we can do that for now and see if user complain. Wdyt?
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.
ok let's keep it for now. We'll see if we do the same for iterators at some point
This update modifies the WorkflowDiagramCanvasBase component to include the starting position of a connection when initiating a drag. It captures the mouse event's coordinates relative to the container, allowing for more precise connection handling. Additionally, a minimum drag distance is introduced to prevent accidental connections from small movements, improving the user experience and interaction accuracy.
This update adds logic to the getWorkflowPreviousSteps utility to handle IF_ELSE step types. It checks if the current step is part of any branches in the potential parent step, improving the accuracy of previous step retrieval in workflows. This change enhances the functionality of the utility while maintaining existing behavior.
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.
1 issue found across 2 files (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="packages/twenty-front/src/modules/workflow/workflow-diagram/components/WorkflowDiagramCanvasBase.tsx">
<violation number="1" location="packages/twenty-front/src/modules/workflow/workflow-diagram/components/WorkflowDiagramCanvasBase.tsx:509">
P1: Touch events are silently ignored, breaking touch device support. When a `TouchEvent` is passed, `mouseEvent` becomes `null` and the function returns early without setting `connectionStartInfo`. Consider handling touch events by extracting coordinates from `event.touches[0]`.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
.../twenty-front/src/modules/workflow/workflow-diagram/components/WorkflowDiagramCanvasBase.tsx
Outdated
Show resolved
Hide resolved
| ? steps.find((step) => step.id === stepId) | ||
| : undefined; | ||
|
|
||
| if ( | ||
| isDefined(stepToDelete) && | ||
| isDefined(steps) && | ||
| stepToDelete.type === 'IF_ELSE' | ||
| ) { | ||
| const emptyChildStepIds = getEmptyChildStepIds({ | ||
| ifElseAction: stepToDelete as WorkflowIfElseAction, | ||
| allSteps: steps, | ||
| }); | ||
|
|
||
| for (const emptyChildStepId of emptyChildStepIds) { | ||
| await deleteWorkflowVersionStep({ | ||
| workflowVersionId, | ||
| stepId: emptyChildStepId, | ||
| }); | ||
| } | ||
|
|
||
| if (emptyChildStepIds.length > 0) { | ||
| deleteStepsOutputSchema({ | ||
| stepIds: emptyChildStepIds, | ||
| workflowVersionId, | ||
| }); | ||
| } | ||
| } | ||
|
|
||
| await deleteWorkflowVersionStep({ | ||
| workflowVersionId, | ||
| stepId, |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
…mponents/WorkflowDiagramCanvasBase.tsx Co-authored-by: cubic-dev-ai[bot] <191113872+cubic-dev-ai[bot]@users.noreply.github.com>
This update enhances the WorkflowDiagramCanvasBase component by restructuring the event handling logic for connection initiation. The code now improves readability and maintains functionality by ensuring that the client coordinates are clearly defined before proceeding with the connection process.
…omponents This update adds unique keys to the WorkflowStepFilterGroupColumn and WorkflowStepFilterColumn components within the WorkflowIfElseBranchEditor. This change improves React's rendering performance and helps prevent potential issues with component reordering.
| startPosition: { | ||
| x: clientX - bounds.left, | ||
| y: clientY - bounds.top, | ||
| }, |
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 is this useful ? can't we simply deactivate the handle when the node is an if-else?
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.
The startPosition is needed to differentiate between a simple click on the handle and an actual drag (when the user drags from the handle to create a new node). Without tracking the start position, we can’t measure the drag distance, and every handle click would end up creating a node. This behavior applies to all node types, not just if-else.
| return !!potentialParentStep.settings.input.branches?.some((branch) => | ||
| branch.nextStepIds?.includes(currentStep.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.
👌
| .map((step) => { | ||
| if ( | ||
| step.type === WorkflowActionType.IF_ELSE && | ||
| isWorkflowIfElseAction(step) |
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.
ok let's keep it for now. We'll see if we do the same for iterators at some point
| const result = await mutate({ | ||
| variables: { input }, | ||
| awaitRefetchQueries: true, | ||
| refetchQueries: [ |
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.
let's keep it for now
thomtrp
left a comment
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.
LGTM
1 similar comment
|
Thanks @abdulrahmancodes for your contribution! |

No description provided.