Feat(agentflow) Node version detection and version upgrade support #6347
Conversation
There was a problem hiding this comment.
Code Review
This pull request implements a node versioning system to identify and upgrade outdated nodes in the Agentflow canvas. It introduces version tracking, warning badges for deprecated or old nodes, and a 'Sync Nodes' header action. Review feedback focuses on improving the robustness of the stale edge detection logic by using edge.target to avoid potential runtime errors and fragile string splitting. Additionally, the reviewer suggests optimizing the node data upgrade process by consolidating input property loops.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a node versioning and synchronization system to detect and upgrade outdated nodes. Key additions include utility functions for version comparison, a 'Sync Nodes' button in the header, and UI indicators for version warnings. Feedback identifies a logic error in version checking that could cause persistent 'outdated' states, performance issues with status re-calculations during node drags, and a recommendation to use Sets for more efficient edge filtering.
| export function isNodeOutdated(nodeData: NodeData, componentNode: NodeDataSchema): boolean { | ||
| if (!nodeData.version) return true | ||
| return componentNode.version !== undefined && componentNode.version > nodeData.version | ||
| } |
There was a problem hiding this comment.
The current logic in isNodeOutdated can lead to an infinite 'outdated' state if the component definition itself lacks a version or has version 0. Specifically, if nodeData.version is missing or 0, it returns true regardless of the componentNode.version. If the component also has no version, the 'upgraded' node will still be considered outdated, causing the 'Sync Nodes' button to persist or trigger redundant updates.
It is safer to check if a target version exists first and then compare. Using a nullish check (== null) is consistent with the repository's standard idiom for checking both null and undefined.
| export function isNodeOutdated(nodeData: NodeData, componentNode: NodeDataSchema): boolean { | |
| if (!nodeData.version) return true | |
| return componentNode.version !== undefined && componentNode.version > nodeData.version | |
| } | |
| export function isNodeOutdated(nodeData: NodeData, componentNode: NodeDataSchema): boolean { | |
| if (componentNode.version == null) return false | |
| if (nodeData.version == null) return true | |
| return componentNode.version > nodeData.version | |
| } |
References
- In JavaScript/TypeScript, use loose equality (== null) as a standard idiom for a 'nullish' check that covers both null and undefined.
There was a problem hiding this comment.
isNodeOutdated was deliberately set to return false for missing nodeData.version so the Sync Nodes FAB doesn't appear for no-version nodes matching V2's implementation
| const hasOutdatedNodes = useMemo( | ||
| () => | ||
| state.nodes.some((node) => { | ||
| const cn = state.componentNodes.find((c) => c.name === node.data.name) | ||
| return cn ? isNodeOutdated(node.data, cn) : false | ||
| }), | ||
| [state.nodes, state.componentNodes] | ||
| ) |
There was a problem hiding this comment.
This useMemo re-calculates hasOutdatedNodes on every change to state.nodes. Since state.nodes is synced from ReactFlow on every drag event, this O(N x M) calculation (where M is the number of component types) runs during every frame of a node drag. For large flows, this can cause noticeable UI stutter.
Consider optimizing this by converting state.componentNodes into a Map for O(1) lookups, or by using a more stable dependency that only changes when node versions or names change.
There was a problem hiding this comment.
Map has to be rebuilt for each drag - optimization does not offer any significant gain
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a node versioning and synchronization system to the Agentflow canvas. It enables the detection of outdated nodes by comparing their stored version against the latest component schema, displaying warning badges and a 'Sync Nodes' action button when updates are available. The implementation includes core utility functions for upgrading node data while preserving user configurations, state management updates in the context and reducer, and a new demo example. Feedback focuses on optimizing performance for node lookups using Maps, ensuring robust error handling for missing version data, and preventing potential infinite loops or redundant notifications during state updates.
| export function isNodeOutdated(nodeData: NodeData, componentNode: NodeDataSchema): boolean { | ||
| if (nodeData.version === undefined || componentNode.version === undefined) return false | ||
| return componentNode.version > nodeData.version | ||
| } | ||
|
|
||
| export function getNodeVersionWarning(nodeData: NodeData, componentNode: NodeDataSchema): string | null { | ||
| if (nodeData.version === undefined && componentNode.version !== undefined) { | ||
| return `Node outdated\nUpdate to latest version ${componentNode.version}` | ||
| } | ||
| if (nodeData.version !== undefined && componentNode.version !== undefined && componentNode.version > nodeData.version) { | ||
| return `Node version ${nodeData.version} outdated\nUpdate to latest version ${componentNode.version}` | ||
| } |
There was a problem hiding this comment.
There is an inconsistency between isNodeOutdated and getNodeVersionWarning regarding nodes with a missing version field. To promote fail-fast behavior and ensure data integrity when handling external data, consider throwing an error when version information is missing instead of silently skipping the check or defaulting to a value.
export function isNodeOutdated(nodeData: NodeData, componentNode: NodeDataSchema): boolean {
if (componentNode.version === undefined) return false
if (nodeData.version === undefined) {
throw new Error('Node version is missing from data source')
}
return componentNode.version > nodeData.version
}References
- When handling potentially invalid data from external sources, prefer throwing an error for invalid input types rather than silently returning a default or empty value.
There was a problem hiding this comment.
isNodeOutdated was deliberately set to return false for missing nodeData.version so the Sync Nodes FAB doesn't appear for no-version nodes matching V2's implementation
| const hasOutdatedNodes = useMemo( | ||
| () => | ||
| state.nodes.some((node) => { | ||
| const cn = state.componentNodes.find((c) => c.name === node.data.name) | ||
| return cn ? isNodeOutdated(node.data, cn) : false | ||
| }), | ||
| [state.nodes, state.componentNodes] | ||
| ) |
There was a problem hiding this comment.
hasOutdatedNodes is re-calculated on every change to state.nodes (including dragging). Since it performs a .find() on componentNodes for every node, this is componentNodes to optimize this to
const hasOutdatedNodes = useMemo(() => {
const componentMap = new Map(state.componentNodes.map((c) => [c.name, c]))
return state.nodes.some((node) => {
const cn = componentMap.get(node.data.name)
return cn ? isNodeOutdated(node.data, cn) : false
})
}, [state.nodes, state.componentNodes])
There was a problem hiding this comment.
Map has to be rebuilt for each drag - optimization does not offer any significant gain
| useEffect(() => { | ||
| if (!isNodesLoading && availableNodes.length > 0) { | ||
| setComponentNodes(availableNodes) | ||
| } | ||
| }, [availableNodes, isNodesLoading, setComponentNodes]) |
There was a problem hiding this comment.
This useEffect dispatches SET_COMPONENT_NODES whenever availableNodes changes. If the useFlowNodes hook returns a new array reference on every render, this will trigger a state update, causing a re-render of AgentflowCanvas, and potentially leading to an infinite loop. Consider adding a check to ensure the content of availableNodes has actually changed before dispatching.
There was a problem hiding this comment.
availableNodes is a useState so it will not be recomputed on every render
|
Re: scope — demo app and public API surface First off, thanks for tackling this — node versioning is a great feature to bring over to the SDK, and the utils port + reducer slice + auto FAB + badges + tests all look clean and faithful to legacy. 🙌 A couple of high-level thoughts on how this lands as an SDK port specifically, that I think would make the change even tighter: 1. The 2. The new public API surface goes a bit beyond legacy parity. Legacy V2 ( Suggested trim to keep the version-sync concern fully internal, matching legacy:
Really nice work overall — just want to trim the public surface and the demo before merging so we ship the smallest correct version of this. 🚀 |
…iseAI/Flowise into feat-agentflow-node-version
FLOWISE-411
Changes Made
getStaleEdgesAfterUpgrade) from the V2 canvas into the AgentFlow SDK
Current Behavior mimicking legacy UI
does not appear. The node cannot be auto-upgraded without a baseline version to compare against.
FAB appears on the canvas. Clicking it upgrades all outdated nodes in one action, preserving user-configured inputs,
credentials, and labels.
Demo:
Ndoe.versioning.fix.recording.mov