Open
Conversation
MayaCrmi
requested review from
Lina-AbuYousef,
Nirsisr,
odaiodeh,
sfiresht and
yhabushi79
Contributor
📝 WalkthroughWalkthroughAdds blueprint edit/save flow: backend PUT endpoint and service error translation; client API for update; large frontend changes to load, reconstruct, render, and update blueprints (edit-mode state, UI edit buttons, graph reconstruction hook, and wiring through pages/hooks/components). Changes
Sequence DiagramsequenceDiagram
participant User as User
participant UI as AgenticWorkflows
participant Builder as NewGraph
participant Hook as useGraphLogic
participant Loader as useLoadBlueprint
participant API as ClientAPI
participant Server as Backend
User->>UI: Click edit on workflow
UI->>UI: set editingBlueprintId
UI->>Builder: open with editBlueprintId
Builder->>Hook: init with editBlueprintId
Hook->>Loader: loadBlueprintForEditing(blueprintId)
Loader->>API: getBlueprintInfo(blueprintId)
API->>Server: GET /blueprints/info/{id}
Server-->>API: spec_dict
API-->>Loader: spec_dict
Loader->>Loader: reconstructBlueprintGraph(spec_dict)
Loader-->>Hook: ReconstructedGraph (nodes, edges, yamlFlow)
Hook->>Builder: populate editor state (isEditMode=true)
User->>Builder: modify graph
User->>Builder: click "Update Workflow"
Builder->>Hook: onSave (blueprintRaw)
Hook->>API: updateBlueprint(blueprintId, blueprintRaw)
API->>Server: PUT /blueprints/blueprint.update
Server-->>API: 200 OK / error
API-->>Hook: response
Hook->>Builder: onSaveComplete / show feedback
Builder->>UI: navigate back / clear editing state
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
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. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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 |
This comment was marked as outdated.
This comment was marked as outdated.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
Here is the code review for the UnifAI Edit Workflow Pull Request. 🔍 SummaryThis PR implements the "Edit Workflow" functionality, allowing users to load existing blueprints into the
Overall, the separation of the reconstruction logic into a new hook is a strong architectural choice, preventing further bloating of the 🧩 File-by-file feedback[DOMAIN: BACKEND] multi-agent/api/flask/endpoints/blueprints.py
[DOMAIN: BACKEND] multi-agent/blueprints/service.py
[DOMAIN: UI] ui/client/src/hooks/use-load-blueprint.ts
[DOMAIN: UI] ui/client/src/hooks/use-graph-logic.ts
[DOMAIN: UI] ui/client/src/components/agentic-ai/graphs/SaveBlueprintModal.tsx
[DOMAIN: UI] ui/client/src/pages/AgenticWorkflows.tsx
[DOMAIN: UI] ui/client/src/workspace/NewGraph.tsx
🛠 Suggested Improvements1. Preserve Selection on Cancel (UI) // ui/client/src/pages/AgenticWorkflows.tsx
const handleBackToFlowConfig = useCallback((savedBlueprint?: SavedBlueprintInfo) => {
// Store the ID before clearing state
const wasEditingId = editingBlueprintId;
setShowGraphBuilder(false);
setEditingBlueprintId(null);
if (savedBlueprint?.blueprintId) {
// ... existing logic for saved flow ...
} else if (wasEditingId) {
// 🛠️ UX Fix: If cancelling edit, keep the original flow selected
// Note: You might need to ensure 'projects' or flow list is available to find the full object,
// or just rely on the ID if WorkflowsPanel handles id-based selection.
// If not, leaving it null is safer than passing a partial object, but inconsistent.
} else {
setSelectedFlow(null);
}
}, [editingBlueprintId]);2. Type Safety for Workspace Data import { BuildingBlock } from "@/types/graph";
// ...
workspaceData: {
// ...
category: "nodes", // ⚠️ Consider using a constant or enum if available
// ...
} as BuildingBlock['workspaceData']✅ What's Good
✍️ Suggested Commit Message |
MayaCrmi
commented
Feb 25, 2026
| @@ -278,15 +258,48 @@ export const useGraphLogic = (options: UseGraphLogicOptions = {}) => { | |||
| const updatedPlan = prevFlow.plan | |||
| .filter((step) => step.uid !== nodeId) | |||
| .map((step) => { | |||
Collaborator
Author
There was a problem hiding this comment.
Please read explanation for this in the description. To see the bug this is fixing try and build a graph say with an Orchestrator connected bidirectionally to two nodes and then delete one of those nodes and you'll see you don't get validation even though you should've, since the deletion doesn't remove the node properly.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@multi-agent/blueprints/service.py`:
- Around line 52-53: The current except block re-raises BlueprintNotFoundError
from a caught KeyError without explicit chaining; change the handler to "except
KeyError as err:" and re-raise with explicit suppression of the original context
to avoid leaking repository internals by using "raise
BlueprintNotFoundError(blueprint_id) from None" (update the except block around
BlueprintNotFoundError/KeyError in service.py).
In `@ui/client/src/hooks/use-graph-logic.ts`:
- Around line 640-642: Replace the one-shot boolean guard
blueprintLoadedRef.current with a ref that tracks the last loaded blueprint ID:
change blueprintLoadedRef to something like lastLoadedBlueprintIdRef and update
the guard in the useGraphLogic hook to return early when
lastLoadedBlueprintIdRef.current === editBlueprintId (also include checks for
!editBlueprintId and isLoadingBlocks), then set lastLoadedBlueprintIdRef.current
= editBlueprintId after loading; apply the same ID-based guard changes to the
other block-loading logic around the 668-680 region so new editBlueprintId
values trigger reloads and avoid saving stale canvas data with the wrong
editBlueprintId.
In `@ui/client/src/hooks/use-load-blueprint.ts`:
- Around line 331-335: Normalize step.exit_condition by calling stripRef once
into a local variable (e.g., normalizedRid = stripRef(step.exit_condition)) and
use that normalizedRid when calling findBlockByRid and when comparing against
stripped specConditions (replace the current findBlockByRid(step.exit_condition)
and stripRef(c.rid) === step.exit_condition check to use normalizedRid), so
lookups succeed whether exit_condition includes a $ref prefix or not.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
multi-agent/api/flask/endpoints/blueprints.pymulti-agent/blueprints/service.pyui/client/src/components/agentic-ai/graphs/GraphHeader.tsxui/client/src/hooks/use-graph-logic.tsui/client/src/hooks/use-load-blueprint.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- ui/client/src/components/agentic-ai/graphs/GraphHeader.tsx
- multi-agent/api/flask/endpoints/blueprints.py
multi-agent/blueprints/service.py
Outdated
Comment on lines
52
to
53
| except KeyError: | ||
| raise BlueprintNotFoundError(blueprint_id) |
Contributor
There was a problem hiding this comment.
Chain the re-raised exception per PEP 3134 / Ruff B904.
Raising inside an except clause without explicit chaining sets __context__ implicitly but leaves __cause__ unset, making tracebacks ambiguous. Since this is a clean domain-boundary translation (repository KeyError → service BlueprintNotFoundError), suppress the chain with from None to avoid leaking repository internals, or chain with from err to preserve the full traceback.
🐛 Proposed fix (suppress internal chain)
- except KeyError:
- raise BlueprintNotFoundError(blueprint_id)
+ except KeyError as err:
+ raise BlueprintNotFoundError(blueprint_id) from NoneOr, to preserve the full traceback for debugging:
- except KeyError:
- raise BlueprintNotFoundError(blueprint_id)
+ except KeyError as err:
+ raise BlueprintNotFoundError(blueprint_id) from err📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| except KeyError: | |
| raise BlueprintNotFoundError(blueprint_id) | |
| except KeyError as err: | |
| raise BlueprintNotFoundError(blueprint_id) from None |
🧰 Tools
🪛 Ruff (0.15.2)
[warning] 53-53: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@multi-agent/blueprints/service.py` around lines 52 - 53, The current except
block re-raises BlueprintNotFoundError from a caught KeyError without explicit
chaining; change the handler to "except KeyError as err:" and re-raise with
explicit suppression of the original context to avoid leaking repository
internals by using "raise BlueprintNotFoundError(blueprint_id) from None"
(update the except block around BlueprintNotFoundError/KeyError in service.py).
Comment on lines
+640
to
+642
| if (!editBlueprintId || isLoadingBlocks || blueprintLoadedRef.current) return; | ||
| blueprintLoadedRef.current = true; | ||
|
|
Contributor
There was a problem hiding this comment.
Track loaded blueprint by ID instead of a boolean guard.
At Line 640 and Line 641, blueprintLoadedRef.current is a one-shot lock. If editBlueprintId changes while this hook stays mounted, the new blueprint is never loaded, but save still uses the new ID at Line 1110. That can overwrite the wrong blueprint with stale canvas data.
🔧 Proposed fix
- const blueprintLoadedRef = useRef(false);
+ const loadedBlueprintIdRef = useRef<string | null>(null);
useEffect(() => {
- if (!editBlueprintId || isLoadingBlocks || blueprintLoadedRef.current) return;
- blueprintLoadedRef.current = true;
+ if (!editBlueprintId || isLoadingBlocks) return;
+ if (loadedBlueprintIdRef.current === editBlueprintId) return;
+ loadedBlueprintIdRef.current = editBlueprintId;
loadBlueprintForEditing(
editBlueprintId,
allBlocksData,
conditionsData,
conditionEdgeColor,
)
@@
.catch((err) => {
console.error("Failed to load blueprint for editing:", err);
+ loadedBlueprintIdRef.current = null;
setIsEditMode(false);
setEditBlueprintName("");
setEditBlueprintDescription("");
toast({
@@
initializeDefaultNodes();
});
}, [editBlueprintId, isLoadingBlocks]);
+
+ useEffect(() => {
+ if (!editBlueprintId) {
+ loadedBlueprintIdRef.current = null;
+ }
+ }, [editBlueprintId]);Also applies to: 668-680
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ui/client/src/hooks/use-graph-logic.ts` around lines 640 - 642, Replace the
one-shot boolean guard blueprintLoadedRef.current with a ref that tracks the
last loaded blueprint ID: change blueprintLoadedRef to something like
lastLoadedBlueprintIdRef and update the guard in the useGraphLogic hook to
return early when lastLoadedBlueprintIdRef.current === editBlueprintId (also
include checks for !editBlueprintId and isLoadingBlocks), then set
lastLoadedBlueprintIdRef.current = editBlueprintId after loading; apply the same
ID-based guard changes to the other block-loading logic around the 668-680
region so new editBlueprintId values trigger reloads and avoid saving stale
canvas data with the wrong editBlueprintId.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
ui/client/src/hooks/use-load-blueprint.ts (1)
8-28: Consider tighteninganytypes in exported interfaces.
config?: anyandbranches?: anyare part of the public API surface exported from this module. Downstream consumers that pattern-match on these fields have no type guidance. At minimum,configcould beRecord<string, unknown>andbranchescould beRecord<string, string>.♻️ Proposed refinement
export interface YamlFlowPlanStep { uid: string; node: string; after?: string | string[] | null; - branches?: any; + branches?: Record<string, string>; exit_condition?: string; } export interface YamlFlowNode { rid: string; name: string; type?: string; - config?: any; + config?: Record<string, unknown>; } export interface YamlFlowCondition { rid: string; name: string; type?: string; - config?: any; + config?: Record<string, unknown>; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/client/src/hooks/use-load-blueprint.ts` around lines 8 - 28, The exported interfaces YamlFlowNode and YamlFlowCondition currently use config?: any and YamlFlowPlanStep uses config?: any and branches?: any; replace these broad any types with stricter shapes (e.g. config?: Record<string, unknown> for node/condition/step and branches?: Record<string, string> for YamlFlowPlanStep) to provide downstream consumers type guidance while preserving optionality; update the type annotations for config and branches in the interfaces YamlFlowNode, YamlFlowCondition, and YamlFlowPlanStep accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ui/client/src/hooks/use-load-blueprint.ts`:
- Around line 409-426: The conditional edge pushed into reactFlowEdges (in the
block that creates an edge with id `${step.uid}-${targetUid}-${branch}` and
data.isConditional) omits width/height on its markerEnd, causing inconsistent
arrow size; update that markerEnd object (the same place where
MarkerType.ArrowClosed is set) to include width: 20 and height: 20 so it matches
the regular edges' marker size.
---
Nitpick comments:
In `@ui/client/src/hooks/use-load-blueprint.ts`:
- Around line 8-28: The exported interfaces YamlFlowNode and YamlFlowCondition
currently use config?: any and YamlFlowPlanStep uses config?: any and branches?:
any; replace these broad any types with stricter shapes (e.g. config?:
Record<string, unknown> for node/condition/step and branches?: Record<string,
string> for YamlFlowPlanStep) to provide downstream consumers type guidance
while preserving optionality; update the type annotations for config and
branches in the interfaces YamlFlowNode, YamlFlowCondition, and YamlFlowPlanStep
accordingly.
Comment on lines
+409
to
+426
| reactFlowEdges.push({ | ||
| id: `${step.uid}-${targetUid}-${branch}`, | ||
| source: step.uid, | ||
| target: targetUid as string, | ||
| type: "custom", | ||
| style: { strokeDasharray: "5,5", stroke: conditionEdgeColor }, | ||
| markerEnd: { | ||
| type: MarkerType.ArrowClosed, | ||
| color: conditionEdgeColor, | ||
| }, | ||
| data: { | ||
| branch: String(branch), | ||
| isConditional: true, | ||
| }, | ||
| label: String(branch), | ||
| }); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Conditional edge markerEnd is missing width/height, causing an inconsistent arrow size.
Regular edges (lines 393–398) explicitly set width: 20, height: 20 on their markerEnd. The conditional edge markerEnd omits these, so the arrowhead will render at the reactflow default size rather than matching the regular edge style.
🐛 Proposed fix
reactFlowEdges.push({
id: `${step.uid}-${targetUid}-${branch}`,
source: step.uid,
target: targetUid as string,
type: "custom",
style: { strokeDasharray: "5,5", stroke: conditionEdgeColor },
markerEnd: {
type: MarkerType.ArrowClosed,
color: conditionEdgeColor,
+ width: 20,
+ height: 20,
},🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ui/client/src/hooks/use-load-blueprint.ts` around lines 409 - 426, The
conditional edge pushed into reactFlowEdges (in the block that creates an edge
with id `${step.uid}-${targetUid}-${branch}` and data.isConditional) omits
width/height on its markerEnd, causing inconsistent arrow size; update that
markerEnd object (the same place where MarkerType.ArrowClosed is set) to include
width: 20 and height: 20 so it matches the regular edges' marker size.
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.
Summary
This PR adds the ability to edit an existing blueprint from the Agentic Workflows page. Clicking the new edit (pencil) button on a workflow loads it back into the graph canvas, where the user can modify it and save the updated version.
Important Changes
New file: use-load-blueprint.ts
use-graph-logic.ts
WorkflowsPanel.tsx / AgentFlowGraph.tsx
GraphHeader.tsx
SaveBlueprintModal.tsx
Delete Node Changes Clarification
The changes in the deleteNode logic are needed to fix a bug that was already there before this new feature. The edit flow exposed the issue because loading a saved blueprint creates a fully connected graph, with after as arrays and multiple branch targets. Deleting a middle node in that structure left dangling references in the YAML.
The fix makes deleteNode consistent with the cleanup logic already used in deleteEdge: it removes the deleted node’s ID from after arrays (collapsing to a string or removing when empty), deletes branch entries that target the node (and clears exit_condition if no branches remain), uses updatedNodes instead of prevFlow.nodes so the YAML actually updates, and spreads ...prevFlow in all setYamlFlow calls so name and description aren’t accidentally dropped.
Addressing Concerns About YAML & Graph Integrity
This edit flow is built as a clean, lossless round-trip. When a blueprint is opened for editing, we load the original spec_dict from the backend and copy its nodes, plan, and conditions directly into yamlFlow. That same yamlFlow object is what we use when generating YAML on save. The React Flow graph is just a visual layer using that data - it doesn’t recreate or reinterpret the blueprint. So all $ref pointers, after/branch connections, exit_condition mappings, and node configs stay exactly as they were.
Edits made in the UI (adding nodes, connecting edges, attaching conditions) go through the same setYamlFlow logic used when building a new graph. There’s no separate “edit mode” mutation path.
The only real difference is what happens at save time - in edit mode, we call PUT /blueprints/blueprint.update, which updates the existing document in place - keeping the same ID, metadata, and sharing settings. This is unlike the new-build mode, where we call POST /blueprints/blueprint.save.
Summary by CodeRabbit
New Features
Bug Fixes