fix: make not connected edges be cleared when entering flow#8678
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe changes introduce a new effect in the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant GenericNode
participant reactflowUtils
User->>GenericNode: Render with data.node.outputs and data.selected_output
GenericNode->>GenericNode: useEffect triggers on outputs/selected_output change
alt No selected_output
GenericNode->>GenericNode: handleSelectOutput(first selected output or null)
end
GenericNode->>reactflowUtils: cleanEdges called (e.g., during resetFlow)
reactflowUtils->>reactflowUtils: Prioritize selected_output, else fallback to first selected output
Possibly related PRs
Suggested labels
Suggested reviewers
✨ Finishing Touches🧪 Generate Unit Tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/frontend/src/CustomNodes/GenericNode/index.tsx (1)
328-333: IncludehandleSelectOutputin the dependency array.While the logic is correct for auto-selecting outputs, the effect should include
handleSelectOutputin its dependency array for completeness, even though it's memoized withuseCallback.useEffect(() => { if (data?.selected_output) return; handleSelectOutput( data.node?.outputs?.find((output) => output.selected) || null, ); - }, [data.node?.outputs, data?.selected_output]); + }, [data.node?.outputs, data?.selected_output, handleSelectOutput]);This follows React's exhaustive-deps ESLint rule and makes the dependencies explicit, improving code maintainability.
src/frontend/src/utils/reactflowUtils.ts (1)
141-148: Review the fallback logic for output selection clarity.The current fallback logic uses an unusual pattern: filtering selected outputs, taking only the first one with
slice(0, 1), then calling.find()on the resulting single-element array. This could miss valid outputs if the first selected output doesn't match the required name but a later selected output does.Consider this scenario:
- Node has outputs:
[{name: "A", selected: true}, {name: "B", selected: true}]- Required name is "B"
- Current logic: Gets first selected output "A", checks if name matches "B", returns undefined
- Result: Edge gets removed even though a valid selected output "B" exists
Consider these alternatives for better clarity:
Option 1: Find the selected output that matches the name
- const output = sourceNode.data.selected_output - ? sourceNode.data.node!.outputs?.find( - (output) => output.name === sourceNode.data.selected_output, - ) - : sourceNode.data - .node!.outputs?.filter((output) => output.selected) - .slice(0, 1) - .find((output) => output.name === name); + const output = sourceNode.data.selected_output + ? sourceNode.data.node!.outputs?.find( + (output) => output.name === sourceNode.data.selected_output, + ) + : sourceNode.data.node!.outputs?.find( + (output) => output.selected && output.name === name + );Option 2: If the intent is to use only the first selected output
- const output = sourceNode.data.selected_output - ? sourceNode.data.node!.outputs?.find( - (output) => output.name === sourceNode.data.selected_output, - ) - : sourceNode.data - .node!.outputs?.filter((output) => output.selected) - .slice(0, 1) - .find((output) => output.name === name); + const output = sourceNode.data.selected_output + ? sourceNode.data.node!.outputs?.find( + (output) => output.name === sourceNode.data.selected_output, + ) + : (() => { + const firstSelected = sourceNode.data.node!.outputs?.find((output) => output.selected); + return firstSelected?.name === name ? firstSelected : undefined; + })();
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/frontend/src/CustomNodes/GenericNode/index.tsx(1 hunks)src/frontend/src/stores/flowStore.ts(1 hunks)src/frontend/src/utils/reactflowUtils.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
`src/frontend/**/*.{ts,tsx,js,jsx}`: All React and TypeScript/JavaScript source files for the frontend must reside under src/frontend/ and use .ts, .tsx, .js, or .jsx extensions.
src/frontend/**/*.{ts,tsx,js,jsx}: All React and TypeScript/JavaScript source files for the frontend must reside under src/frontend/ and use .ts, .tsx, .js, or .jsx extensions.
src/frontend/src/CustomNodes/GenericNode/index.tsxsrc/frontend/src/stores/flowStore.tssrc/frontend/src/utils/reactflowUtils.ts
`src/frontend/src/stores/**/*`: Zustand state management stores must be placed under src/frontend/src/stores/.
src/frontend/src/stores/**/*: Zustand state management stores must be placed under src/frontend/src/stores/.
src/frontend/src/stores/flowStore.ts
`src/frontend/src/utils/**/*`: Utility functions must be placed under src/frontend/src/utils/.
src/frontend/src/utils/**/*: Utility functions must be placed under src/frontend/src/utils/.
src/frontend/src/utils/reactflowUtils.ts
🔇 Additional comments (1)
src/frontend/src/stores/flowStore.ts (1)
229-229: LGTM! Improved consistency by using cleaned edges.This change ensures that
unselectAllNodesEdgesoperates on the cleaned edges (newEdges) rather than the raw edges from flow data. This maintains consistency with the cleaned edges that are actually stored in the state, eliminating potential discrepancies between what gets unselected and what gets persisted.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/frontend/src/CustomNodes/GenericNode/index.tsx(1 hunks)src/frontend/src/stores/flowStore.ts(1 hunks)src/frontend/src/utils/reactflowUtils.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
`src/frontend/**/*.{ts,tsx,js,jsx}`: All React and TypeScript/JavaScript source files for the frontend must reside under src/frontend/ and use .ts, .tsx, .js, or .jsx extensions.
src/frontend/**/*.{ts,tsx,js,jsx}: All React and TypeScript/JavaScript source files for the frontend must reside under src/frontend/ and use .ts, .tsx, .js, or .jsx extensions.
src/frontend/src/utils/reactflowUtils.tssrc/frontend/src/CustomNodes/GenericNode/index.tsxsrc/frontend/src/stores/flowStore.ts
`src/frontend/src/utils/**/*`: Utility functions must be placed under src/frontend/src/utils/.
src/frontend/src/utils/**/*: Utility functions must be placed under src/frontend/src/utils/.
src/frontend/src/utils/reactflowUtils.ts
`src/frontend/src/stores/**/*`: Zustand state management stores must be placed under src/frontend/src/stores/.
src/frontend/src/stores/**/*: Zustand state management stores must be placed under src/frontend/src/stores/.
src/frontend/src/stores/flowStore.ts
🔇 Additional comments (1)
src/frontend/src/stores/flowStore.ts (1)
229-229: Good consistency improvement.Using
newEdges(the cleaned edges) instead of the originaledgesensures that the unselection logic operates on the same edge data that will be used in the flow state. This maintains consistency throughout the reset process.
…-ai#8678) * Fixed output not cleaning edges * Added useEffect to select first output if none is selected * updated useeffect condition * Fixed edges connected being cleared * added handleSelectOutput to dependencies * Fixed loop disconnecting
This pull request includes updates to improve edge handling and node/edge selection logic in the flow management system. The changes ensure more precise behavior when interacting with nodes and edges and enhance the maintainability of the code.
Improvements to edge handling:
src/frontend/src/utils/reactflowUtils.ts: Updated thecleanEdgesfunction to handle cases where aselected_outputis defined for a node. Ifselected_outputis present, it finds the corresponding output; otherwise, it defaults to filtering outputs marked as selected, ensuring only one output is considered.Updates to node/edge selection logic:
src/frontend/src/stores/flowStore.ts: Modified theunselectAllNodesEdgesfunction call to usenewEdgesinstead ofedges, ensuring the updated edge list is used during the unselection process.Summary by CodeRabbit