feat(frontend/builder): Add sub-graph update UX #11631
Conversation
github-project-automation
bot
moved this to 🆕 Needs initial review
in AutoGPT development kanban
✅ Deploy Preview for auto-gpt-docs-dev canceled.
|
WalkthroughImplements in-graph sub-agent updates with a resolution mode: detects compatible vs incompatible updates, stages incompatible updates, tracks and visually flags broken connections, and applies pending updates once broken edges are resolved. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI as SubAgentUpdateBar / Node UI
participant Hook as useSubAgentUpdate
participant Store as nodeStore / graphStore
participant Flow as FlowEditor / Flow
User->>UI: Click "Update"
UI->>Hook: Request update check (availableGraphs, schemas, connections)
Hook->>Hook: Compare versions & compute incompatibilities
alt Compatible
Hook-->>UI: isCompatible = true
UI->>Flow: applyUpdate(nodeID, newHardcodedValues)
Flow->>Store: updateNodeData(nodeID, values)
Store-->>UI: Node updated
else Incompatible
Hook-->>UI: isCompatible = false + incompatibilities
UI->>UI: Open IncompatibilityDialog / show ResolutionModeBar
User->>UI: Confirm "Update Anyway"
UI->>Flow: enterResolutionMode(nodeID, incompatibilities, brokenEdgeIds, pendingUpdate)
Flow->>Store: setNodeResolutionMode & setBrokenEdgeIDs
Store-->>UI: Render broken visuals (red/dashed edges, disabled handles)
loop User fixes connections
User->>UI: Remove/adjust broken edges
UI->>Store: edge state updated
end
alt all broken edges cleared
Store->>Flow: notify cleared
Flow->>Flow: applyPendingUpdate(nodeID)
Flow->>Store: clearResolutionState(nodeID)
Store-->>UI: Exit resolution mode
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (8)autogpt_platform/frontend/**/*.{ts,tsx}📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
autogpt_platform/frontend/**/*.{ts,tsx,json}📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
autogpt_platform/frontend/src/**/*.{ts,tsx}📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
autogpt_platform/frontend/**/*.{js,ts,jsx,tsx}📄 CodeRabbit inference engine (AGENTS.md)
Files:
autogpt_platform/frontend/**📄 CodeRabbit inference engine (autogpt_platform/CLAUDE.md)
Files:
autogpt_platform/frontend/src/**/*.tsx📄 CodeRabbit inference engine (autogpt_platform/CLAUDE.md)
Files:
autogpt_platform/frontend/src/app/**/*.tsx📄 CodeRabbit inference engine (autogpt_platform/CLAUDE.md)
Files:
autogpt_platform/frontend/**/*.{ts,tsx,css}📄 CodeRabbit inference engine (autogpt_platform/CLAUDE.md)
Files:
🧠 Learnings (8)📚 Learning: 2025-11-25T08:48:33.246ZApplied to files:
📚 Learning: 2025-11-25T08:49:03.583ZApplied to files:
📚 Learning: 2025-11-25T08:49:03.583ZApplied to files:
📚 Learning: 2025-11-25T08:48:33.246ZApplied to files:
📚 Learning: 2025-11-25T08:49:03.583ZApplied to files:
📚 Learning: 2025-11-25T08:48:33.246ZApplied to files:
📚 Learning: 2025-11-25T08:48:33.246ZApplied to files:
📚 Learning: 2025-11-25T08:49:03.583ZApplied to files:
🧬 Code graph analysis (1)autogpt_platform/frontend/src/app/(platform)/build/components/legacy-builder/BuildActionBar.tsx (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
🔇 Additional comments (1)
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. Comment |
✅ Deploy Preview for auto-gpt-docs canceled.
|
|
Here's the code health analysis summary for commits Analysis Summary
|
|
Thanks for implementing this sub-graph update feature! This will definitely improve the user experience when working with sub-agents in the graph editor. The code looks good with well-structured components for handling updates, incompatibility detection, and resolution mode. I particularly like the clear visual indicators for broken connections and the detailed incompatibility dialog. Before mergingYou need to complete your test plan by verifying all the test scenarios you've outlined:
Once you've completed all the tests and received design approval, please update the checklist to reflect this. Minor notes
Please let us know when you've completed the remaining tests so we can merge this improvement. |
|
Thank you for this PR that addresses the sub-graph update UX! The implementation looks thorough and well-designed. Looking Good
Before MergingYour test plan needs completion:
Please complete the remaining test plan items to ensure this feature works as expected in all scenarios. Once those are checked off, this should be ready to merge. Is there anything you need help with to finish testing these scenarios? |
Pwuts
requested review from
Bentlybro and
Swiftyos
and removed request for
a team
|
Thanks for working on improving the sub-graph update experience! This is a valuable enhancement that will make the builder UX much better. FeedbackYour implementation looks well thought out, with good separation of concerns between the UI components and the core logic in the However, before we can merge this PR:
Once these two items are addressed, the PR should be ready for merging. The code implementation itself looks solid and well-structured. |
…in-graph-without-re-adding
|
Thanks for your PR implementing the sub-graph update UX! This is a valuable improvement that will make updating sub-agents much more user-friendly. The implementation looks thorough - I like how you've added comprehensive UI elements for detecting updates and handling incompatibilities, particularly the resolution mode that highlights broken connections. Before this can be approved, please complete the remaining test item in your checklist:
Once you've tested this scenario and checked it off, this PR should be ready for approval. The implementation itself looks solid, with good separation of concerns between the detection logic and UI components. |
|
Thanks for this well-structured PR implementing the sub-graph update UX! The feature looks comprehensive, with support for both builder versions and thorough handling of compatibility issues. The code changes look good, but I noticed that your checklist has one uncompleted item: "Designer approves of the look". Since this involves significant UI changes with new dialogs, banners, and resolution workflows, design approval appears to be required by your team's process. Before this can be merged, please ensure you've received design approval for the UI elements, and check that item off in the PR description. |
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
| nodesInResolutionMode: new Set<string>(), | ||
| brokenEdgeIDs: new Set<string>(), | ||
| nodeResolutionData: new Map<string, NodeResolutionData>(), | ||
|
|
||
| setNodeResolutionMode: ( | ||
| nodeID: string, | ||
| inResolution: boolean, | ||
| resolutionData?: NodeResolutionData, | ||
| ) => { | ||
| set((state) => { | ||
| const newNodesSet = new Set(state.nodesInResolutionMode); | ||
| const newResolutionDataMap = new Map(state.nodeResolutionData); | ||
|
|
||
| if (inResolution) { | ||
| newNodesSet.add(nodeID); | ||
| if (resolutionData) { | ||
| newResolutionDataMap.set(nodeID, resolutionData); | ||
| } | ||
| } else { | ||
| newNodesSet.delete(nodeID); | ||
| newResolutionDataMap.delete(nodeID); | ||
| } | ||
|
|
||
| return { | ||
| nodesInResolutionMode: newNodesSet, | ||
| nodeResolutionData: newResolutionDataMap, | ||
| }; | ||
| }); | ||
| }, | ||
|
|
||
| isNodeInResolutionMode: (nodeId: string) => { | ||
| return get().nodesInResolutionMode.has(nodeId); | ||
| }, | ||
|
|
||
| getNodeResolutionData: (nodeId: string) => { | ||
| return get().nodeResolutionData.get(nodeId); | ||
| }, | ||
|
|
||
| setBrokenEdgeIDs: (edgeIds: string[]) => { | ||
| set({ brokenEdgeIDs: new Set(edgeIds) }); | ||
| }, | ||
|
|
There was a problem hiding this comment.
Critical: Global brokenEdgeIDs causes conflicts with per-node resolution mode
The brokenEdgeIDs is stored as a single global Set<string> (line 346), but resolution mode is tracked per-node via nodesInResolutionMode: Set<string> (line 345). This creates a critical bug:
- If two nodes enter resolution mode simultaneously, their broken edge IDs will be mixed in the same global set
- When one node completes resolution and calls
setBrokenEdgeIds([]), it clears ALL broken edges, breaking the other node's resolution state - The
setNodeResolutionModefunction (lines 349-373) doesn't clearbrokenEdgeIDswhen exiting resolution mode, leaving stale data
Fix: Change the data structure to track broken edges per node:
brokenEdgeIDs: Map<string, Set<string>>, // nodeId -> Set of broken edge IDs
setBrokenEdgeIDs: (nodeId: string, edgeIds: string[]) => {
set((state) => {
const newMap = new Map(state.brokenEdgeIDs);
newMap.set(nodeId, new Set(edgeIds));
return { brokenEdgeIDs: newMap };
});
},
removeBrokenEdgeID: (nodeId: string, edgeId: string) => {
set((state) => {
const newMap = new Map(state.brokenEdgeIDs);
const nodeSet = new Set(newMap.get(nodeId));
nodeSet.delete(edgeId);
newMap.set(nodeId, nodeSet);
return { brokenEdgeIDs: newMap };
});
},Also update setNodeResolutionMode to clean up node-specific broken edges when exiting resolution mode.
| nodesInResolutionMode: new Set<string>(), | |
| brokenEdgeIDs: new Set<string>(), | |
| nodeResolutionData: new Map<string, NodeResolutionData>(), | |
| setNodeResolutionMode: ( | |
| nodeID: string, | |
| inResolution: boolean, | |
| resolutionData?: NodeResolutionData, | |
| ) => { | |
| set((state) => { | |
| const newNodesSet = new Set(state.nodesInResolutionMode); | |
| const newResolutionDataMap = new Map(state.nodeResolutionData); | |
| if (inResolution) { | |
| newNodesSet.add(nodeID); | |
| if (resolutionData) { | |
| newResolutionDataMap.set(nodeID, resolutionData); | |
| } | |
| } else { | |
| newNodesSet.delete(nodeID); | |
| newResolutionDataMap.delete(nodeID); | |
| } | |
| return { | |
| nodesInResolutionMode: newNodesSet, | |
| nodeResolutionData: newResolutionDataMap, | |
| }; | |
| }); | |
| }, | |
| isNodeInResolutionMode: (nodeId: string) => { | |
| return get().nodesInResolutionMode.has(nodeId); | |
| }, | |
| getNodeResolutionData: (nodeId: string) => { | |
| return get().nodeResolutionData.get(nodeId); | |
| }, | |
| setBrokenEdgeIDs: (edgeIds: string[]) => { | |
| set({ brokenEdgeIDs: new Set(edgeIds) }); | |
| }, | |
| nodesInResolutionMode: new Set<string>(), | |
| brokenEdgeIDs: new Map<string, Set<string>>(), | |
| nodeResolutionData: new Map<string, NodeResolutionData>(), | |
| setNodeResolutionMode: ( | |
| nodeID: string, | |
| inResolution: boolean, | |
| resolutionData?: NodeResolutionData, | |
| ) => { | |
| set((state) => { | |
| const newNodesSet = new Set(state.nodesInResolutionMode); | |
| const newResolutionDataMap = new Map(state.nodeResolutionData); | |
| const newBrokenEdgeIDs = new Map(state.brokenEdgeIDs); | |
| if (inResolution) { | |
| newNodesSet.add(nodeID); | |
| if (resolutionData) { | |
| newResolutionDataMap.set(nodeID, resolutionData); | |
| } | |
| } else { | |
| newNodesSet.delete(nodeID); | |
| newResolutionDataMap.delete(nodeID); | |
| newBrokenEdgeIDs.delete(nodeID); // Clean up broken edges when exiting resolution mode | |
| } | |
| return { | |
| nodesInResolutionMode: newNodesSet, | |
| nodeResolutionData: newResolutionDataMap, | |
| brokenEdgeIDs: newBrokenEdgeIDs, | |
| }; | |
| }); | |
| }, | |
| isNodeInResolutionMode: (nodeId: string) => { | |
| return get().nodesInResolutionMode.has(nodeId); | |
| }, | |
| getNodeResolutionData: (nodeId: string) => { | |
| return get().nodeResolutionData.get(nodeId); | |
| }, | |
| setBrokenEdgeIDs: (nodeId: string, edgeIds: string[]) => { | |
| set((state) => { | |
| const newMap = new Map(state.brokenEdgeIDs); | |
| newMap.set(nodeId, new Set(edgeIds)); | |
| return { brokenEdgeIDs: newMap }; | |
| }); | |
| }, | |
| removeBrokenEdgeID: (nodeId: string, edgeId: string) => { | |
| set((state) => { | |
| const newMap = new Map(state.brokenEdgeIDs); | |
| const nodeSet = new Set(newMap.get(nodeId) || []); | |
| nodeSet.delete(edgeId); | |
| newMap.set(nodeId, nodeSet); | |
| return { brokenEdgeIDs: newMap }; | |
| }); | |
| }, |
Spotted by Graphite Agent
Is this helpful? React 👍 or 👎 to let us know.
Abhi1992002
left a comment
Abhi1992002
left a comment
There was a problem hiding this comment.
Nice 🙌
I’ve added a review for the new builder (haven’t checked the old one).
| // Fetch all available graphs for sub-agent update detection | ||
| const { data: availableGraphs } = useGetV1ListUserGraphs({ | ||
| query: { select: okData }, | ||
| }); | ||
|
|
There was a problem hiding this comment.
what if a user has access to 100's of graphs but is using only 2–3 subgraphs in the current agent. Don’t you think fetching all available graphs will increase backend load (and cost) and also hurt frontend performance?
Instead, could we create a new endpoint that returns only the user’s graphs based on a list of graph_ids? We could first fetch the details of the current graph, extract all the agent blocks, and then use their graph_ids to fetch only the required graphs.
I know this approach would lose some parallelism—we’d need to wait for the main graph to load before checking compatibility—but it should improve overall performance. We could show a loader on the agent node saying “checking compatibility.” WDYT?
There was a problem hiding this comment.
It’s totally fine if you want to handle this in follow-up PRs, since this one is already quite large. [I’m also planning a few changes to reduce loading and saving time, since I’m currently using some brute-force logic in a few places]
...form/frontend/src/app/(platform)/build/components/FlowEditor/nodes/CustomNode/CustomNode.tsx
Outdated
Show resolved
Hide resolved
...form/frontend/src/app/(platform)/build/components/FlowEditor/nodes/CustomNode/CustomNode.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
We can create IncompatibleUpdateDialog folder - and put IncompatibleUpdateDialog.tsx inisde it, as well as types.ts and also a folder named components with 2 files TwoColumnSection.tsx and SingleColumnSection
There was a problem hiding this comment.
I think that would hurt rather than help DX. I know it would for me. These are (right now) components that will always be used together, and they are designed to work together.
There was a problem hiding this comment.
Also, in many places we’re not using the Text atom. Can we use it instead of normal elements? It would be good for design consistency.
There was a problem hiding this comment.
Doesn't the default global font suffice for standard body text?
...d/components/FlowEditor/nodes/CustomNode/components/SubAgentUpdate/useSubAgentUpdateState.ts
Outdated
Show resolved
Hide resolved
...d/components/FlowEditor/nodes/CustomNode/components/SubAgentUpdate/useSubAgentUpdateState.ts
Outdated
Show resolved
Hide resolved
...d/components/FlowEditor/nodes/CustomNode/components/SubAgentUpdate/useSubAgentUpdateState.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
nit: I think this component now have enough states —maybe we can create a useFormCreator hook.
...gpt_platform/frontend/src/app/(platform)/build/components/FlowEditor/nodes/OutputHandler.tsx
Outdated
Show resolved
Hide resolved
github-project-automation
bot
moved this from 🆕 Needs initial review
to 🚧 Needs work
in AutoGPT development kanban
autogpt_platform/frontend/src/app/(platform)/build/hooks/useSubAgentUpdate.ts
Outdated
Show resolved
Hide resolved
autogpt_platform/frontend/src/app/(platform)/build/stores/nodeStore.ts
Outdated
Show resolved
Hide resolved
autogpt_platform/frontend/src/app/(platform)/build/components/FlowEditor/nodes/FormCreator.tsx
Outdated
Show resolved
Hide resolved
...t_platform/frontend/src/components/renderers/input-renderer/fields/AnyOfField/AnyOfField.tsx
Outdated
Show resolved
Hide resolved
...tend/src/app/(platform)/build/components/legacy-builder/CustomNode/IncompatibilityDialog.tsx
Show resolved
Hide resolved
...tend/src/app/(platform)/build/components/legacy-builder/CustomNode/IncompatibilityDialog.tsx
Show resolved
Hide resolved
...omponents/FlowEditor/nodes/CustomNode/components/SubAgentUpdate/IncompatibleUpdateDialog.tsx
Outdated
Show resolved
Hide resolved
…in-graph-without-re-adding
github-actions
bot
added
the
conflicts
|
This pull request has conflicts with the base branch, please resolve those so we can evaluate the pull request. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In
@autogpt_platform/frontend/src/app/(platform)/build/components/legacy-builder/BuildActionBar.tsx:
- Line 4: In BuildActionBar replace lucide-react icons with Phosphor
equivalents: change the import statement that currently imports AlertTriangle
and LogOut from "lucide-react" to import Warning and SignOut (or SignOutSimple
if used elsewhere) from "phosphor-react", then update JSX usages in the
BuildActionBar component to render <Warning .../> instead of <AlertTriangle
.../> and <SignOut .../> instead of <LogOut .../> so only Phosphor Icons are
used.
In
@autogpt_platform/frontend/src/app/(platform)/build/components/legacy-builder/CustomNode/IncompatibilityDialog.tsx:
- Around line 2-10: This file imports legacy UI components (Dialog,
DialogContent, DialogDescription, DialogFooter, DialogHeader, DialogTitle,
Button); replace those legacy imports with the design-system Dialog and Button
components (use the project’s Dialog molecule and Button atom equivalents) and
update any prop names/usage to match the design-system API; use the
IncompatibleUpdateDialog.tsx implementation as the reference for import names
and prop usage to ensure compatibility, then run the frontend build/tests to
verify no API mismatches remain.
In @autogpt_platform/frontend/src/lib/autogpt-server-api/types.ts:
- Around line 248-249: The change touches deprecated backend API types in
types.ts (the lines with "type?: never;" and "const?: never;"); do not edit this
legacy file—revert any modifications and instead migrate the consumer code to
use the generated API hooks under "@/app/api/__generated__/endpoints/"
(regenerate with "pnpm generate:api" if needed) so the schema change is applied
via the generated client rather than the deprecated src/lib/autogpt-server-api
layer.
🧹 Nitpick comments (17)
autogpt_platform/frontend/src/app/(platform)/build/components/legacy-builder/CustomNode/CustomNode.tsx (2)
139-143: Throwing during render may cause issues.Throwing an error during the render phase when
builderContextis missing can cause React error boundaries to trigger and may result in a poor developer experience. Consider using a more graceful fallback or moving this check.♻️ Suggested alternative
- if (!builderContext) { - throw new Error( - "BuilderContext consumer must be inside FlowEditor component", - ); - } + // Early return with null or a fallback if context is missing + // This shouldn't happen in practice, but provides a safer fallback + if (!builderContext) { + console.error("BuilderContext consumer must be inside FlowEditor component"); + return null; + }Alternatively, keep the throw but move it to an effect or use a custom hook that handles the context check.
204-215: Direct mutation ofdataprop during render.The code directly assigns to
data.inputSchemaanddata.outputSchema, which are properties of thedataprop. While this creates new objects for the properties, mutating props during render is an anti-pattern in React and can lead to subtle bugs or unexpected behavior.Consider computing the merged schemas as local variables and using them in the render without mutating the prop object.
♻️ Suggested refactor
+ // Compute merged schemas without mutating data + const mergedInputSchema = { + ...newInputSchema, + properties: mergedInputProps, + }; + const mergedOutputSchema = { + ...newOutputSchema, + properties: mergedOutputProps, + }; - data.inputSchema = { - ...newInputSchema, - properties: mergedInputProps, - }; - data.outputSchema = { - ...newOutputSchema, - properties: mergedOutputProps, - }; + // Then use mergedInputSchema and mergedOutputSchema in renderYou would need to track these as local variables and use them instead of
data.inputSchema/data.outputSchemathroughout the component.autogpt_platform/frontend/src/app/(platform)/build/components/legacy-builder/useCopyPaste.ts (1)
115-123: Non-null assertions on handle properties.The non-null assertions (
!) onsourceHandleandtargetHandleassume these properties are always defined. While this is typically true for edges in the builder context, it could cause runtime errors if an edge somehow lacks these properties.Consider adding a defensive filter or defaulting to empty strings:
♻️ Suggested safer approach
.map( (edge: Edge): ConnectedEdge => ({ id: edge.id, source: edge.source, target: edge.target, - sourceHandle: edge.sourceHandle!, - targetHandle: edge.targetHandle!, + sourceHandle: edge.sourceHandle ?? "", + targetHandle: edge.targetHandle ?? "", }), );Or filter out edges without handles before mapping:
+ .filter( + (edge: Edge) => edge.sourceHandle != null && edge.targetHandle != null, + ) .map( (edge: Edge): ConnectedEdge => ({ id: edge.id, source: edge.source, target: edge.target, - sourceHandle: edge.sourceHandle!, - targetHandle: edge.targetHandle!, + sourceHandle: edge.sourceHandle!, + targetHandle: edge.targetHandle!, }), );autogpt_platform/frontend/src/app/(platform)/build/hooks/useSubAgentUpdate/helpers.ts (1)
64-104: Potential duplicate edge IDs in result.The function checks for missing inputs (lines 76-82) and type mismatches (lines 85-91) separately. If an input name appears in both
missingInputsandinputTypeMismatches, the same edge ID would be pushed twice.While this is unlikely in practice (a missing input shouldn't have a type mismatch), consider using a
SetforbrokenEdgeIDsor checking for existence before pushing:♻️ Suggested fix using Set
export function getBrokenEdgeIDs( connections: EdgeLike[], incompatibilities: IncompatibilityInfo, nodeID: string, ): string[] { - const brokenEdgeIDs: string[] = []; + const brokenEdgeIDs = new Set<string>(); const typeMismatchInputNames = new Set( incompatibilities.inputTypeMismatches.map((m) => m.name), ); connections.forEach((conn) => { // Check if this connection uses a missing input (node is target) if ( conn.target === nodeID && conn.targetHandle && incompatibilities.missingInputs.includes(conn.targetHandle) ) { - brokenEdgeIDs.push(conn.id); + brokenEdgeIDs.add(conn.id); } // Check if this connection uses an input with a type mismatch (node is target) if ( conn.target === nodeID && conn.targetHandle && typeMismatchInputNames.has(conn.targetHandle) ) { - brokenEdgeIDs.push(conn.id); + brokenEdgeIDs.add(conn.id); } // Check if this connection uses a missing output (node is source) if ( conn.source === nodeID && conn.sourceHandle && incompatibilities.missingOutputs.includes(conn.sourceHandle) ) { - brokenEdgeIDs.push(conn.id); + brokenEdgeIDs.add(conn.id); } }); - return brokenEdgeIDs; + return Array.from(brokenEdgeIDs); }autogpt_platform/frontend/src/app/(platform)/build/hooks/useSubAgentUpdate/index.ts (1)
1-2: Barrel file violates coding guidelines.As per coding guidelines: "No barrel files or index.ts re-exports in frontend". This
index.tsfile serves purely as a re-export barrel.Consider removing this file and having consumers import directly from the specific modules:
import { useSubAgentUpdate } from "./useSubAgentUpdate";import { createUpdatedAgentNodeInputs, getBrokenEdgeIDs } from "./helpers";♻️ Suggested approach
Delete this barrel file and update import paths in consuming files to import directly from the source modules.
For example, in
SubAgentUpdateFeature.tsxor other consumers:// Instead of: import { useSubAgentUpdate } from "@/app/(platform)/build/hooks/useSubAgentUpdate"; // Use: import { useSubAgentUpdate } from "@/app/(platform)/build/hooks/useSubAgentUpdate/useSubAgentUpdate";autogpt_platform/frontend/src/app/(platform)/build/components/FlowEditor/nodes/CustomNode/components/SubAgentUpdate/components/ResolutionModeBar.tsx (1)
17-80: Consider extractingrenderIncompatibilitiesoutside the component.The inner function is recreated on every render. Since it only depends on
incompatibilitiesprop, extracting it as a separate function or usinguseCallbackwould improve performance, though the impact is minimal given the component's usage context.♻️ Optional: Extract as standalone function
+function renderIncompatibilities(incompatibilities: IncompatibilityInfo | null) { + if (!incompatibilities) return <span>No incompatibilities</span>; + // ... rest of the logic +} export function ResolutionModeBar({ incompatibilities, }: ResolutionModeBarProps): React.ReactElement { - const renderIncompatibilities = () => { - // ... - };autogpt_platform/frontend/src/app/(platform)/build/components/FlowEditor/handlers/NodeHandle.tsx (2)
12-41: Consider using function declaration for component.Per coding guidelines, components should use function declarations rather than arrow functions. While this is a small component, consistency with the codebase convention would be beneficial.
♻️ Suggested refactor to function declaration
-const NodeHandle = ({ +function NodeHandle({ handleId, isConnected, side, isBroken = false, -}: NodeHandleProps) => { +}: NodeHandleProps) { return ( // ... rest of component ); -}; +}Based on coding guidelines, function declarations are preferred for components.
33-36: Minor: Redundantopacity-100class.The
opacity-100class has no effect since it's the default opacity. It can be safely removed.♻️ Suggested cleanup
className={cn( - "opacity-100", isBroken ? "text-red-500" : "text-gray-400", )}autogpt_platform/frontend/src/app/(platform)/build/components/legacy-builder/CustomNode/IncompatibilityDialog.tsx (1)
118-124: Button variant and custom styling conflict.The button uses
variant="destructive"but overrides the colors withbg-amber-600 hover:bg-amber-700. This is a semantic mismatch—destructive typically implies red/danger. Consider usingvariant="primary"or a custom variant if amber is the intended warning color.♻️ Suggested fix
<Button - variant="destructive" + variant="default" onClick={onConfirm} - className="bg-amber-600 hover:bg-amber-700" + className="border-amber-700 bg-amber-600 hover:bg-amber-700" >autogpt_platform/frontend/src/app/(platform)/build/components/FlowEditor/nodes/FormCreator.tsx (1)
24-38: Consider memoizinghandleChangewithuseCallback.The
handleChangefunction is recreated on every render, which could cause unnecessary re-renders of the FormRenderer. Since this is a memoized component, stabilizing the callback would improve the memoization benefit.♻️ Suggested improvement
+import React, { useCallback } from "react"; // ... - const handleChange = ({ formData }: any) => { + const handleChange = useCallback(({ formData }: any) => { if ("credentials" in formData && !formData.credentials?.id) { delete formData.credentials; } const updatedValues = uiType === BlockUIType.AGENT ? { ...getHardCodedValues(nodeId), inputs: formData, } : formData; updateNodeData(nodeId, { hardcodedValues: updatedValues }); - }; + }, [uiType, nodeId, getHardCodedValues, updateNodeData]);autogpt_platform/frontend/src/app/(platform)/build/components/FlowEditor/nodes/CustomNode/components/SubAgentUpdate/useSubAgentUpdateState.ts (3)
33-52: Remove unnecessaryuseShallowwrappers for single primitive/function selectors.
useShallowis designed for shallow comparison of objects/arrays to prevent unnecessary re-renders. When selecting a single function reference (likeupdateNodeData,setNodeResolutionMode, etc.),useShallowadds overhead without benefit since function references from Zustand are already stable.Suggested simplification
- const updateNodeData = useNodeStore( - useShallow((state) => state.updateNodeData), - ); - const setNodeResolutionMode = useNodeStore( - useShallow((state) => state.setNodeResolutionMode), - ); - const isNodeInResolutionMode = useNodeStore( - useShallow((state) => state.isNodeInResolutionMode), - ); - const setBrokenEdgeIDs = useNodeStore( - useShallow((state) => state.setBrokenEdgeIDs), - ); + const updateNodeData = useNodeStore((state) => state.updateNodeData); + const setNodeResolutionMode = useNodeStore((state) => state.setNodeResolutionMode); + const isNodeInResolutionMode = useNodeStore((state) => state.isNodeInResolutionMode); + const setBrokenEdgeIDs = useNodeStore((state) => state.setBrokenEdgeIDs); // ... same for getNodeResolutionData
110-153: Consider wrappinghandleConfirmIncompatibleUpdateinuseCallback.This function is returned from the hook and may be passed as a prop to child components. Without
useCallback, a new function reference is created on every render, potentially causing unnecessary re-renders in consuming components.Suggested fix
- function handleConfirmIncompatibleUpdate() { + const handleConfirmIncompatibleUpdate = useCallback(() => { if (!updateInfo.latestGraph || !updateInfo.incompatibilities) return; // ... rest of implementation setShowIncompatibilityDialog(false); - } + }, [ + updateInfo.latestGraph, + updateInfo.incompatibilities, + nodeData.hardcodedValues, + connectedEdges, + currentInputSchema, + currentOutputSchema, + setBrokenEdgeIDs, + setNodeResolutionMode, + nodeID, + ]);
177-183: Stable functions can be removed from the dependency array.As noted in previous review feedback,
setNodeResolutionModeandupdateNodeDataare stable store functions that won't change between renders. Including them doesn't cause bugs but adds unnecessary noise to the dependency array.autogpt_platform/frontend/src/app/(platform)/build/hooks/useSubAgentUpdate/useSubAgentUpdate.ts (1)
30-33: Potential issue with non-null assertion onlatestGraph.version.The non-null assertion
latestGraph.version!assumesversionis always defined whenlatestGraphexists. Ifversioncould beundefinedornull, this comparison would behave unexpectedly (comparingundefined > numberreturnsfalsein JavaScript, which happens to be safe here, but the assertion is misleading).Consider adding explicit handling:
Safer version check
const hasUpdate = useMemo(() => { - if (!latestGraph || graphVersion === undefined) return false; - return latestGraph.version! > graphVersion; + if (!latestGraph || graphVersion === undefined || latestGraph.version === undefined) return false; + return latestGraph.version > graphVersion; }, [latestGraph, graphVersion]);autogpt_platform/frontend/src/app/(platform)/build/components/legacy-builder/CustomNode/SubAgentUpdateBar.tsx (2)
21-27: Arrow function components instead of function declarations.Per coding guidelines: "Use function declarations for components and handlers (not arrow functions)."
Function declaration style
-export const SubAgentUpdateBar: React.FC<SubAgentUpdateBarProps> = ({ - currentVersion, - // ... -}) => { +export function SubAgentUpdateBar({ + currentVersion, + // ... +}: SubAgentUpdateBarProps) { -const ResolutionModeBar: React.FC<ResolutionModeBarProps> = ({ - incompatibilities, -}) => { +function ResolutionModeBar({ + incompatibilities, +}: ResolutionModeBarProps) {Also applies to: 73-75
73-128: ResolutionModeBar duplicates logic from the v2 implementation.There's a similar
ResolutionModeBarcomponent in the v2 builder (autogpt_platform/frontend/src/app/(platform)/build/components/FlowEditor/nodes/CustomNode/components/SubAgentUpdate/components/ResolutionModeBar.tsx). Consider extracting this into a shared component to avoid duplication and ensure consistent behavior across both builders.autogpt_platform/frontend/src/app/(platform)/build/components/legacy-builder/Flow/Flow.tsx (1)
77-83: Consider co-locatingResolutionModeStatetype with the new builder's types.The
ResolutionModeStatetype defined here duplicates the concept from the new builder'sNodeResolutionDatain nodeStore. While the legacy builder needs its own state management, having similar but different types could lead to drift. Consider documenting the relationship or extracting shared types.
autogpt_platform/frontend/src/app/(platform)/build/components/legacy-builder/BuildActionBar.tsx
Outdated
Show resolved
Hide resolved
| import { | ||
| Dialog, | ||
| DialogContent, | ||
| DialogDescription, | ||
| DialogFooter, | ||
| DialogHeader, | ||
| DialogTitle, | ||
| } from "@/components/__legacy__/ui/dialog"; | ||
| import { Button } from "@/components/__legacy__/ui/button"; |
There was a problem hiding this comment.
Using legacy components violates coding guidelines.
This file imports from @/components/__legacy__/ui/dialog and @/components/__legacy__/ui/button. Per coding guidelines, legacy components should never be used in frontend code. Use the design system components from src/components/ (atoms, molecules, organisms) instead.
Consider using:
@/components/molecules/Dialog/Dialoginstead of legacy dialog@/components/atoms/Button/Buttoninstead of legacy button
The newer IncompatibleUpdateDialog.tsx in the FlowEditor path already uses the correct design system components and could serve as a reference.
🤖 Prompt for AI Agents
In
@autogpt_platform/frontend/src/app/(platform)/build/components/legacy-builder/CustomNode/IncompatibilityDialog.tsx
around lines 2 - 10, This file imports legacy UI components (Dialog,
DialogContent, DialogDescription, DialogFooter, DialogHeader, DialogTitle,
Button); replace those legacy imports with the design-system Dialog and Button
components (use the project’s Dialog molecule and Button atom equivalents) and
update any prop names/usage to match the design-system API; use the
IncompatibleUpdateDialog.tsx implementation as the reference for import names
and prop usage to ensure compatibility, then run the frontend build/tests to
verify no API mismatches remain.
| DialogTitle, | ||
| } from "@/components/__legacy__/ui/dialog"; | ||
| import { Button } from "@/components/__legacy__/ui/button"; | ||
| import { AlertTriangle, XCircle, PlusCircle } from "lucide-react"; |
There was a problem hiding this comment.
Use Phosphor Icons instead of lucide-react.
Per coding guidelines, only Phosphor Icons (@phosphor-icons/react) should be used in the frontend. Replace lucide-react icons with their Phosphor equivalents.
♻️ Suggested icon replacements
-import { AlertTriangle, XCircle, PlusCircle } from "lucide-react";
+import { WarningIcon, XCircleIcon, PlusCircleIcon } from "@phosphor-icons/react";Then update usages to use weight="fill" prop for filled appearance:
<WarningIcon className="h-5 w-5 text-amber-500" weight="fill" />
<XCircleIcon className="h-4 w-4 text-red-500" weight="fill" />
<PlusCircleIcon className="h-4 w-4 text-green-500" weight="fill" />Based on coding guidelines, only Phosphor Icons should be used.
Committable suggestion skipped: line range outside the PR's diff.
![sentry[bot]](https://avatars.githubusercontent.com/u/1396951?s=60&v=4){kind=small}
|
|
||
| // Update available sub-graphs in store for sub-agent update detection | ||
| useEffect(() => { | ||
| if (availableGraphs) { | ||
| setAvailableSubGraphs(availableGraphs); | ||
| } | ||
| }, [availableGraphs, setAvailableSubGraphs]); | ||
|
|
||
| // adding nodes | ||
| useEffect(() => { | ||
| if (customNodes.length > 0) { |
There was a problem hiding this comment.
Bug: Global resolution state is not cleared when switching between flows, causing nodes in the new flow to incorrectly inherit stale resolution mode from the previous flow.
Severity: HIGH | Confidence: High
🔍 Detailed Analysis
The resolution state, managed in the nodeStore, is not reset when a user navigates between different flows. While setNodes([]) is called to clear the current nodes, the associated resolution data (nodesInResolutionMode, brokenEdgeIDs, etc.) persists. If a newly loaded flow contains a node with an ID that was previously in resolution mode, that node will incorrectly render with stale "broken connection" indicators and incompatibility data from the old flow. This happens because the cleanup function clearResolutionState() exists but is never invoked on flow change or component unmount.
💡 Suggested Fix
Call the existing clearResolutionState() function from the nodeStore within the cleanup effect in useFlow.ts. This will ensure that all resolution-related state is properly reset when the user switches between flows, preventing stale data from affecting the new flow's state.
🤖 Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location:
autogpt_platform/frontend/src/app/(platform)/build/components/FlowEditor/Flow/useFlow.ts#L126-L138
Potential issue: The resolution state, managed in the `nodeStore`, is not reset when a
user navigates between different flows. While `setNodes([])` is called to clear the
current nodes, the associated resolution data (`nodesInResolutionMode`, `brokenEdgeIDs`,
etc.) persists. If a newly loaded flow contains a node with an ID that was previously in
resolution mode, that node will incorrectly render with stale "broken connection"
indicators and incompatibility data from the old flow. This happens because the cleanup
function `clearResolutionState()` exists but is never invoked on flow change or
component unmount.
Did we get this right? 👍 / 👎 to inform future reviews.
Reference ID: 8298818
OPEN-2743: Ability to Update Sub-Agents in Graph (Without Re-Adding)
Updating sub-graphs is a cumbersome experience at the moment, this should help. :)
Demo in Builder v2:
2026-01-02.15.03.vivaldi.mp4
2026-01-02.17.00.vivaldi.mp4
Changes 🏗️
CustomNode>IncompatibilityDialog+SubAgentUpdateBarsub-componentsSubAgentUpdateFeature+ResolutionModeBar+IncompatibleUpdateDialog+useSubAgentUpdateStatesub-componentsuseSubAgentUpdatehookChecklist 📋
For code changes:
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Style
✏️ Tip: You can customize this high-level summary in your review settings.