Fix: clean up iteration child nodes and edges on delete (#13889)#14033
Open
owldev127 wants to merge 9 commits intoinfiniflow:mainfrom
Open
Fix: clean up iteration child nodes and edges on delete (#13889)#14033owldev127 wants to merge 9 commits intoinfiniflow:mainfrom
owldev127 wants to merge 9 commits intoinfiniflow:mainfrom
Conversation
dosubot
bot
added
the
size:L
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughExpand deletion logic to remove iteration node subtrees and associated edges by traversing graph store nodes, update selection/click state accordingly, and extend the before-delete hook to consult the global graph state. Tests added for hook and store behavior. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as UI (delete action)
participant Hook as useBeforeDelete
participant Store as Graph Store
participant Utils as collectDeletionNodeIds
UI->>Hook: request before-delete for nodeId
Hook->>Store: read graphNodes, graphEdges, getNode, getOperatorTypeFromId
Hook->>Utils: collectDeletionNodeIds(graphNodes, nodeId) (if Operator.Iteration)
Utils-->>Hook: descendant node IDs
Hook->>Store: filter graphEdges by deletion node ID set
Hook-->>UI: return deletion result (nodes, edges)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
web/src/pages/agent/store.test.ts (1)
5-25: Minor:baseNodeis referenced before its definition.While function declarations are hoisted in JavaScript/TypeScript, defining
baseNodebeforecreateNodewould improve readability.♻️ Suggested reordering
+function baseNode(id: string, label: Operator) { + return { + id, + type: 'ragNode', + position: { x: 0, y: 0 }, + data: { + label, + name: id, + form: {}, + }, + }; +} + const createNode = ( id: string, label: Operator, options: Partial<ReturnType<typeof baseNode>> = {}, ) => ({ ...baseNode(id, label), ...options, }); - -function baseNode(id: string, label: Operator) { - return { - id, - type: 'ragNode', - position: { x: 0, y: 0 }, - data: { - label, - name: id, - form: {}, - }, - }; -}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/pages/agent/store.test.ts` around lines 5 - 25, Move the baseNode function above createNode to avoid referencing baseNode before its definition; locate the baseNode and createNode declarations in the file (functions named baseNode and createNode) and reorder them so baseNode is declared first, leaving the implementations unchanged to preserve behavior.web/src/pages/agent/hooks/use-before-delete.tsx (1)
54-100: Edge filtering logic at lines 54-89 is dead code.The intermediate
toBeDeletedEdgescomputed in lines 54-70 and extended in lines 85-89 is completely replaced at line 96. The reassignmenttoBeDeletedEdges = graphEdges.filter(...)discards all prior edge filtering work.If the intent is to use the
graphEdges-based filtering as the authoritative source (which appears correct for comprehensive edge cleanup), consider removing or simplifying the dead code.♻️ Proposed simplification
- let toBeDeletedEdges = edges.filter((edge) => { - const sourceType = getOperatorTypeFromId(edge.source) as Operator; - const downStreamNodes = nodes.filter((x) => x.id === edge.target); - - // This edge does not need to be deleted, the range of edges that do not need to be deleted is smaller, so consider the case where it does not need to be deleted - if ( - UndeletableNodes.includes(sourceType) && // Upstream node is Begin or IterationStart - downStreamNodes.length === 0 // Downstream node does not exist in the nodes to be deleted - ) { - if (!nodes.some((x) => x.id === edge.source)) { - return true; // Can be deleted - } - return false; // Cannot be deleted - } - - return true; - }); - // Delete the agent and tool nodes downstream of the agent node if (nodes.some(agentPredicate)) { nodes.filter(agentPredicate).forEach((node) => { - const { downstreamAgentAndToolEdges, downstreamAgentAndToolNodeIds } = + const { downstreamAgentAndToolNodeIds } = deleteAllDownstreamAgentsAndTool(node.id, edges); downstreamAgentAndToolNodeIds.forEach((nodeId) => { const currentNode = getNode(nodeId); if (toBeDeletedNodes.every((x) => x.id !== nodeId) && currentNode) { toBeDeletedNodes.push(currentNode); } }); - - downstreamAgentAndToolEdges.forEach((edge) => { - if (toBeDeletedEdges.every((x) => x.id !== edge.id)) { - toBeDeletedEdges.push(edge); - } - }); }, []); } const toBeDeletedNodeIdSet = new Set( toBeDeletedNodes.map((node) => node.id), ); - toBeDeletedEdges = graphEdges.filter( + const toBeDeletedEdges = graphEdges.filter( (edge) => toBeDeletedNodeIdSet.has(edge.source) || toBeDeletedNodeIdSet.has(edge.target), );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/pages/agent/hooks/use-before-delete.tsx` around lines 54 - 100, The initial computation and incremental pushes into toBeDeletedEdges (the filter starting at toBeDeletedEdges = edges.filter(...) and the downstreamAgentAndToolEdges.forEach(...) inside the agentPredicate block) are dead because you always overwrite toBeDeletedEdges later with toBeDeletedEdges = graphEdges.filter(...); either remove the initial edges.filter(...) and the downstreamAgentAndToolEdges.forEach(...) logic, or instead merge them by replacing the final reassignment with a union of graphEdges.filter(...) and the previously accumulated toBeDeletedEdges so the results from deleteAllDownstreamAgentsAndTool (and the initial source-type check) are preserved; update references to toBeDeletedEdges, nodes, edges, agentPredicate, deleteAllDownstreamAgentsAndTool, graphEdges, and toBeDeletedNodes accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@web/src/pages/agent/hooks/use-before-delete.tsx`:
- Around line 54-100: The initial computation and incremental pushes into
toBeDeletedEdges (the filter starting at toBeDeletedEdges = edges.filter(...)
and the downstreamAgentAndToolEdges.forEach(...) inside the agentPredicate
block) are dead because you always overwrite toBeDeletedEdges later with
toBeDeletedEdges = graphEdges.filter(...); either remove the initial
edges.filter(...) and the downstreamAgentAndToolEdges.forEach(...) logic, or
instead merge them by replacing the final reassignment with a union of
graphEdges.filter(...) and the previously accumulated toBeDeletedEdges so the
results from deleteAllDownstreamAgentsAndTool (and the initial source-type
check) are preserved; update references to toBeDeletedEdges, nodes, edges,
agentPredicate, deleteAllDownstreamAgentsAndTool, graphEdges, and
toBeDeletedNodes accordingly.
In `@web/src/pages/agent/store.test.ts`:
- Around line 5-25: Move the baseNode function above createNode to avoid
referencing baseNode before its definition; locate the baseNode and createNode
declarations in the file (functions named baseNode and createNode) and reorder
them so baseNode is declared first, leaving the implementations unchanged to
preserve behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 34f05d35-fa34-4707-b57b-421fd4b84a9e
📒 Files selected for processing (4)
web/src/pages/agent/hooks/use-before-delete.test.tsxweb/src/pages/agent/hooks/use-before-delete.tsxweb/src/pages/agent/store.test.tsweb/src/pages/agent/store.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What problem does this PR solve?
Fixes a workflow editor bug where deleting an Iteration Box could leave orphan child nodes and dangling edges in client state. Those stale references could be exported with the workflow and later cause rendering errors, broken connections, and unstable editing behavior.
Root Cause
Iteration deletion logic only removed the container, its direct children, and some internal edges. It did not consistently remove the full descendant subtree or all edges connected to deleted child nodes, and the keyboard delete path was not expanded to include Iteration descendants.
Type of change