-
Notifications
You must be signed in to change notification settings - Fork 795
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
Fix loop block label update bug #1496
Conversation
…src/' <!-- ELLIPSIS_HIDDEN --> > [!IMPORTANT] > Fixes label update bug in `getUpdatedNodesAfterLabelUpdateForParameterKeys` by correcting `loopVariableReference` update logic in `workflowEditorUtils.ts`. > > - **Bug Fix**: > - Fixes label update bug in `getUpdatedNodesAfterLabelUpdateForParameterKeys` in `workflowEditorUtils.ts`. > - Corrects `loopVariableReference` update logic to match new label if it matches old label's output parameter key. > > <sup>This description was created by </sup>[<img alt="Ellipsis" src="https://img.shields.io/badge/Ellipsis-blue?color=175173">](https://www.ellipsis.dev?ref=Skyvern-AI%2Fskyvern-cloud&utm_source=github&utm_medium=referral)<sup> for 82027835d3d358d18583fff9263bbbe62118900f. It will automatically update as commits are pushed.</sup> <!-- ELLIPSIS_HIDDEN -->
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.
👍 Looks good to me! Incremental review on af92eab in 36 seconds
More details
- Looked at
19
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
1
drafted comments based on config settings.
1. skyvern-frontend/src/routes/workflows/editor/workflowEditorUtils.ts:1197
- Draft comment:
Consider refactoring the repeated logic for updating the label to improve maintainability and reduce redundancy. This logic appears in multiple places, such as lines 1197 and 1209. - Reason this comment was not posted:
Confidence changes required:50%
The PR description indicates a fix for updating loopVariableReference when the label changes. The code correctly updates loopVariableReference if it matches the old label's output parameter key. However, the logic for updating the label itself is repeated in multiple places, which could be refactored for clarity and maintainability.
Workflow ID: wflow_JDzCUaYeZvZ4W7oc
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
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.
👍 Looks good to me! Reviewed everything up to af92eab in 52 seconds
More details
- Looked at
19
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
1
drafted comments based on config settings.
1. skyvern-frontend/src/routes/workflows/editor/workflowEditorUtils.ts:1199
- Draft comment:
Consider updatingloopValue
similarly toloopVariableReference
if it depends on the label.
loopValue:
node.data.loopValue === getOutputParameterKey(oldLabel)
? getOutputParameterKey(newLabel)
: node.data.loopValue,
- Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The code shows a clear refactoring whereloopValue
is being replaced byloopVariableReference
. The comment suggests keeping both, but this would go against the apparent design decision to rename the field. The code is consistently usingloopVariableReference
throughout the file, suggesting this was an intentional change.
Could there be a reason to maintain backward compatibility by keeping both fields? Could removingloopValue
break existing code?
The code appears to be part of a frontend component where maintaining API compatibility is less critical. The consistent use ofloopVariableReference
throughout the file suggests this was a deliberate refactoring decision.
The comment should be deleted as it suggests maintaining redundant fields that would go against the clear refactoring decision made in the code.
Workflow ID: wflow_KEqcKGP500hWWN5I
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
Important
Fixes label update bug in
getUpdatedNodesAfterLabelUpdateForParameterKeys
by correctingloopVariableReference
update logic inworkflowEditorUtils.ts
.getUpdatedNodesAfterLabelUpdateForParameterKeys
inworkflowEditorUtils.ts
.loopVariableReference
update logic to match new label if it matches old label's output parameter key.This description was created by for af92eab. It will automatically update as commits are pushed.