fix(copilot): allow loop-in-loop workflow edits#3723
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
PR SummaryMedium Risk Overview Refactors container handling into shared helpers and makes Adds tests covering loop-in-loop editing, grandchild ID preservation, and descendant/edge cleanup when nested nodes are omitted or replaced. Written by Cursor Bugbot for commit 0fd55db. This will update automatically on new commits. Configure here. |
Greptile SummaryThis PR enables loop-in-loop (and deeper nesting) workflow editing by refactoring the Key changes:
Confidence Score: 3/5
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[handleEditOperation\nparams.nestedNodes?] --> B[mergeNestedNodesForParent\nparentBlockId, nestedNodes]
B --> C{For each incoming\nnestedNode}
C --> D{Match by name in\nexisting children?}
D -- Yes --> E[Update existing block\nsubBlocks / inputs]
E --> F{Existing block is\nloop or parallel?}
F -- Yes --> G[updateLoopOrParallelContainerData]
G --> H{childBlock has\nnestedNodes?}
H -- Yes --> B
H -- No --> C
F -- No --> C
D -- No --> I[createBlockFromParams\nnew child block]
I --> J{New child is\nloop or parallel?}
J -- Yes --> K[applyLoopOrParallelContainerData\nprocessNestedNodesForParent]
J -- No --> C
C -- Done --> L[Remove unmatched\nexisting children\nfrom blocks + edges]
L --> M{⚠️ Removed child is\nloop or parallel?}
M -- Yes --> N[BUG: grandchildren\nnot cleaned up]
M -- No --> O[Done]
Reviews (1): Last reviewed commit: "Fix lint" | Re-trigger Greptile |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Autofix Details
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Removing nested loop orphans its descendant blocks
- Updated nested merge deletion to recursively collect and remove unmatched descendants (and their edges), and added a regression test covering nested container removal.
Or push these changes by commenting:
@cursor push ed6ee626fd
Preview (ed6ee626fd)
diff --git a/apps/sim/lib/copilot/tools/server/workflow/edit-workflow/operations.test.ts b/apps/sim/lib/copilot/tools/server/workflow/edit-workflow/operations.test.ts
--- a/apps/sim/lib/copilot/tools/server/workflow/edit-workflow/operations.test.ts
+++ b/apps/sim/lib/copilot/tools/server/workflow/edit-workflow/operations.test.ts
@@ -408,4 +408,39 @@
const helperBlock = Object.values(state.blocks).find((block: any) => block.name === 'Helper')
expect(helperBlock).toBeDefined()
})
+
+ it('removes descendants when a nested container child is removed', () => {
+ const workflow = makeNestedLoopWorkflow()
+
+ const { state } = applyOperationsToWorkflowState(workflow, [
+ {
+ operation_type: 'edit',
+ block_id: 'outer-loop',
+ params: {
+ nestedNodes: {
+ replacement: {
+ type: 'function',
+ name: 'Replacement Helper',
+ inputs: { code: 'return 1' },
+ },
+ },
+ },
+ },
+ ])
+
+ expect(state.blocks['inner-loop']).toBeUndefined()
+ expect(state.blocks['inner-agent']).toBeUndefined()
+ expect(
+ state.edges.some((edge: any) => edge.source === 'inner-loop' || edge.target === 'inner-loop')
+ ).toBe(false)
+ expect(
+ state.edges.some((edge: any) => edge.source === 'inner-agent' || edge.target === 'inner-agent')
+ ).toBe(false)
+
+ const replacementBlock = Object.values(state.blocks).find(
+ (block: any) => block.name === 'Replacement Helper'
+ ) as any
+ expect(replacementBlock).toBeDefined()
+ expect(replacementBlock.data?.parentId).toBe('outer-loop')
+ })
})
diff --git a/apps/sim/lib/copilot/tools/server/workflow/edit-workflow/operations.ts b/apps/sim/lib/copilot/tools/server/workflow/edit-workflow/operations.ts
--- a/apps/sim/lib/copilot/tools/server/workflow/edit-workflow/operations.ts
+++ b/apps/sim/lib/copilot/tools/server/workflow/edit-workflow/operations.ts
@@ -298,14 +298,23 @@
})
const removedIds = new Set<string>()
+ const findChildrenToRemove = (parentId: string) => {
+ Object.entries(modifiedState.blocks).forEach(([childId, child]: [string, any]) => {
+ if (child.data?.parentId === parentId && !removedIds.has(childId)) {
+ removedIds.add(childId)
+ findChildrenToRemove(childId)
+ }
+ })
+ }
for (const [existingId] of existingChildren) {
if (!matchedExistingIds.has(existingId)) {
- delete modifiedState.blocks[existingId]
removedIds.add(existingId)
+ findChildrenToRemove(existingId)
}
}
if (removedIds.size > 0) {
+ removedIds.forEach((id) => delete modifiedState.blocks[id])
modifiedState.edges = modifiedState.edges.filter(
(edge: any) => !removedIds.has(edge.source) && !removedIds.has(edge.target)
)This Bugbot Autofix run was free. To enable autofix for future PRs, go to the Cursor dashboard.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
|
|
||
| if (childBlock.nestedNodes && (childBlock.type === 'loop' || childBlock.type === 'parallel')) { | ||
| processNestedNodesForParent(childId, childBlock.nestedNodes, ctx) | ||
| } |
There was a problem hiding this comment.
Duplicated block creation logic across two new functions
Low Severity
The "create new child block" path inside mergeNestedNodesForParent (the else branch for unmatched nodes) is a near-exact copy of the body of processNestedNodesForParent: both call createBlockFromParams, conditionally call applyLoopOrParallelContainerData, add to modifiedState.blocks, push deferred connections, and recurse via processNestedNodesForParent. If a bug is fixed in one copy, the other could easily be missed.
Additional Locations (1)
| (existingBlock.type === 'loop' || existingBlock.type === 'parallel') | ||
| ) { | ||
| mergeNestedNodesForParent(existingId, childBlock.nestedNodes, ctx) | ||
| } |
There was a problem hiding this comment.
Credential pre-validation skips deeply nested blocks
Medium Severity
The new recursive mergeNestedNodesForParent and processNestedNodesForParent functions correctly process arbitrarily nested nestedNodes, but preValidateCredentialInputs in validation.ts only iterates one level of nestedNodes. Credentials (e.g., oauth-input IDs) inside a deeply nested block—such as an agent inside an inner loop inside an outer loop—will bypass the ownership pre-validation check that guards against unauthorized credential references.
Additional Locations (1)
Summary
We now support loop in loop. Adjusted edit-workflow to handle this.
processNestedNodeForParents,updateLoopOrParallelContainerDataType of Change
Testing
Tested with loop in loop and loop in loop in loop
Checklist
Screenshots/Videos