Feature/workflowmodel #59
Conversation
- Introduced UserWorkflows component to display user-specific workflows. - Implemented API call to fetch workflows for the logged-in user. - Added error handling and loading states for improved user experience. - Created JSON STRUCTURES.md to document email input structure.
- Revised the description of the Webhook Trigger to enhance understanding of its functionality.
📝 WalkthroughWalkthroughThis PR introduces workflow retrieval and display functionality with enhanced error handling. It adds a new API method for fetching user workflows, creates a workflows listing page, significantly refactors the workflow canvas initialization with defensive programming patterns and centralized error state, registers a webhook trigger, and updates test setup with a stage field. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/web/app/workflows/[id]/page.tsx (2)
207-240: Async API calls are not awaited, breaking error handling.The
api.triggers.updateandapi.nodes.updatecalls inside theforEachloop are async but not awaited. This means:
- The
try-catchwill never catch errors from these callssetError(null)executes immediately before API calls complete- Failed updates result in unhandled promise rejections
🔧 Suggested fix: Await API calls and handle errors properly
- const handleNodesChange = (changes: NodeChange[]) => { - onNodesChange(changes); - try { - changes.forEach((change) => { + const handleNodesChange = async (changes: NodeChange[]) => { + onNodesChange(changes); + + const positionChanges = changes.filter( + (change) => change.type === "position" && change.position + ); + + if (positionChanges.length === 0) return; + + try { + await Promise.all( + positionChanges.map(async (change) => { if (change.type === "position" && change.position) { const changedNode = nodes.find((n) => n.id === change.id); if (changedNode?.data?.nodeType === "trigger") { - api.triggers.update({ + await api.triggers.update({ TriggerId: change.id, Config: { ...(typeof changedNode.data.config === "object" && changedNode.data.config !== null ? changedNode.data.config : {}), position: change.position, }, }); } else { - api.nodes.update({ + await api.nodes.update({ NodeId: change.id, position: change.position, }); } } - }); + }) + ); setError(null); } catch (err: any) {Note: Consider debouncing position updates to avoid excessive API calls during drag operations.
333-365: Stale closure:nodesreference insetEdgescallback may be outdated.The
setEdgescallback referencesnodesfrom the outer closure, butsetNodeswas just called above. React state updates are batched, sonodeshere reflects the state beforesetNodeswas called, leading to potential edge calculation errors.🔧 Suggested fix: Calculate edge data before state updates
+ // Calculate edge data BEFORE updating nodes state + const existingActionNodes = nodes.filter( + (n) => n.data.nodeType === "action" && !n.data.isPlaceholder + ); + const sourceNodeId = + existingActionNodes.length > 0 + ? existingActionNodes[existingActionNodes.length - 1]!.id + : triggerId; + setNodes((prevNodes) => { // Remove any existing action placeholder nodes const filtered = prevNodes.filter( (n) => !(n.data.isPlaceholder && n.data.nodeType === "action") ); return [...filtered, newNode, actionPlaceholder]; }); setEdges((prevEdges) => { // Remove placeholder-targeting edges - const filtered = prevEdges.filter((e) => { - const targetNode = nodes.find((n) => n.id === e.target); - return !( - targetNode?.data.isPlaceholder && - targetNode?.data.nodeType === "action" - ); - }); - - const existingActionNodes = nodes.filter( - (n) => n.data.nodeType === "action" && !n.data.isPlaceholder - ); - - const sourceNodeId = - existingActionNodes.length > 0 - ? existingActionNodes[existingActionNodes.length - 1]!.id - : triggerId; + const filtered = prevEdges.filter( + (e) => !e.target.includes("placeholder") + ); return [ ...filtered,
🤖 Fix all issues with AI agents
In `@apps/web/app/workflows/`[id]/page.tsx:
- Around line 252-278: The local variable `cuont` is a typo and always 0; remove
the `let cuont = 0;` declaration and pass a meaningful stage value to
api.nodes.create (replace `stage: cuont`), e.g. use the computed `nextIndex` (or
another explicit constant like 0 if stage must always be zero); update the call
to api.nodes.create to use `stage: nextIndex` (or `stage: 0`) and delete the
unused `cuont` symbol to fix the typo and remove the unused variable.
In `@apps/web/app/workflows/page.tsx`:
- Around line 25-44: The effect currently only calls fetchWorkflows when userId
is truthy, but loading is left true when userId is falsy; update the useEffect
that contains fetchWorkflows so that when userId is null/undefined you
immediately call setLoading(false) (and optionally setData([]) or
setError(null)) instead of leaving the spinner active; change the logic around
useEffect/userId to explicitly handle the else branch and ensure fetchWorkflows
still manages loading/error when invoked.
In `@JSON` STRUCTURES.md:
- Around line 1-2: The file contains only the placeholder label "Email Input :"
and lacks the intended JSON schema or examples; either populate it with the
concrete JSON structure(s) for the expected "Email Input" (include required
fields, types, example values, and optional fields) or delete the file to avoid
confusion — update the document by replacing the placeholder label with the
complete JSON structure and at least one example payload so reviewers and
consumers can rely on it.
🧹 Nitpick comments (7)
apps/web/app/workflows/[id]/components/ConfigModal.tsx (1)
27-27: Remove commented-out debug code.Commented-out console.log statements should be removed before merging to keep the codebase clean.
🧹 Suggested cleanup
const userId = useAppSelector((state) => state.user.userId) as string; - // console.log("This is the credential Data from config from backend" , config); // Fetch credentials with hook based on node config (google, etc) if appropriateapps/web/app/lib/api.ts (1)
9-17: Consider more descriptive naming and address potential duplication.The implementation is correct, but:
api.user.getis vague - considerapi.user.getWorkflowsto be explicit about what's being fetched.- Similar logic exists in
apps/web/app/workflow/lib/config.ts(per the relevant snippets). Consider consolidating to avoid duplication.♻️ Suggested naming improvement
user: { - get: async () => { + getWorkflows: async () => { return await axios.get(`${BACKEND_URL}/user/workflows`, { withCredentials: true, headers: { "Content-Type": "application/json" }, }) } },apps/web/app/workflows/page.tsx (4)
16-16: Remove unnecessary comment.This comment about a removed import adds noise and should be deleted.
🧹 Suggested cleanup
import { useRouter } from "next/navigation"; -// Removed: import { Router } from "next/router";
18-23: Improve type safety with a Workflow interface.Using
any[]and type assertions reduces type safety. Consider defining a proper interface.♻️ Suggested improvement
+interface Workflow { + id: string; + name?: string; + Name?: string; + description?: string; + config?: Record<string, unknown>; + Config?: Record<string, unknown>; +} + export const UserWorkflows = () => { - const [data, setData] = useState<any[]>([]); + const [data, setData] = useState<Workflow[]>([]); const [loading, setLoading] = useState(true); const [error, setError] = useState<string | null>(null); - const userId = useAppSelector((select) => select.user.userId) as string; + const userId = useAppSelector((select) => select.user.userId);
33-33: Remove debug console.log before merging.Console logging in production code should be removed or replaced with proper logging/monitoring.
🧹 Suggested cleanup
setData(Array.isArray(workflows) ? workflows : []); - console.log("The workflow data is", workflows); } catch (error) {
59-70: Simplify styling by removing redundant inline styles.The inline styles largely duplicate what the Tailwind classes already provide (
w-64 h-64). Consider removing the redundantstyleprop or consolidating.♻️ Suggested simplification
<Card key={workflow.id || workflow.Name} - className="w-64 h-64 flex flex-col justify-between items-stretch rounded-xl shadow-md bg-background" - style={{ - aspectRatio: '1 / 1', - maxWidth: '16rem', - maxHeight: '16rem', - minWidth: '16rem', - minHeight: '16rem', - marginLeft: 'auto', - marginRight: 'auto' - }} + className="w-64 h-64 min-w-64 min-h-64 max-w-64 max-h-64 mx-auto flex flex-col justify-between items-stretch rounded-xl shadow-md bg-background aspect-square" >apps/web/app/workflows/[id]/page.tsx (1)
508-514: Consider consistent styling approach.The Save button mixes Tailwind classes with inline styles. While functional, consider using one approach consistently for maintainability.
♻️ Optional: Use Tailwind only
<button onClick={handleSave} - className=" bottom-4 border bg-white text-black font-bold p-4 z-50 shadow-lg px-12 rounded-2xl" - style={{ position: "fixed", bottom: "1rem", right: "10rem" }} + className="fixed bottom-4 right-40 border bg-white text-black font-bold p-4 z-50 shadow-lg px-12 rounded-2xl" > Save </button>
| let cuont = 0; | ||
| // Calculate next available action node index (excluding placeholders) | ||
| const currentActionNodes = nodes.filter( | ||
| (n) => n.data.nodeType === "action" && !n.data.isPlaceholder | ||
| ); | ||
| const nextIndex = currentActionNodes.length; | ||
|
|
||
| // Place new action node after last action (or at default offset if none) | ||
| const ACTION_NODE_DEFAULT_X = 350; | ||
| const ACTION_NODE_DEFAULT_Y = 400; | ||
| const ACTION_NODE_OFFSET_X = 320; | ||
| const newNodePosition = { | ||
| x: ACTION_NODE_DEFAULT_X + ACTION_NODE_OFFSET_X * nextIndex, | ||
| y: ACTION_NODE_DEFAULT_Y, | ||
| }; | ||
|
|
||
| try { | ||
| const result = await api.nodes.create({ | ||
| Name: action.name, | ||
| AvailableNodeId: action.id, | ||
| Config: { | ||
| CredentialsID: "", | ||
| }, | ||
| WorkflowId: workflowId, | ||
| Position: 1, | ||
| position: newNodePosition, | ||
| stage: cuont, | ||
| }); |
There was a problem hiding this comment.
Typo and unused variable: cuont is always 0.
The variable cuont (likely meant to be "count") is declared but never incremented or modified. It's passed as stage: cuont which will always be 0. If this is intentional, consider using a constant or removing the variable.
🔧 Suggested fix
If the stage should always be 0:
- let cuont = 0;
- // Calculate next available action node index (excluding placeholders)
- const currentActionNodes = nodes.filter(
- (n) => n.data.nodeType === "action" && !n.data.isPlaceholder
- );
- const nextIndex = currentActionNodes.length;
+ // Calculate next available action node index (excluding placeholders)
+ const currentActionNodes = nodes.filter(
+ (n) => n.data.nodeType === "action" && !n.data.isPlaceholder
+ );
+ const nextIndex = currentActionNodes.length;
...
- stage: cuont,
+ stage: 0,If the stage should reflect the node's position in the workflow:
- let cuont = 0;
...
- stage: cuont,
+ stage: nextIndex + 1, // or appropriate stage value📝 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.
| let cuont = 0; | |
| // Calculate next available action node index (excluding placeholders) | |
| const currentActionNodes = nodes.filter( | |
| (n) => n.data.nodeType === "action" && !n.data.isPlaceholder | |
| ); | |
| const nextIndex = currentActionNodes.length; | |
| // Place new action node after last action (or at default offset if none) | |
| const ACTION_NODE_DEFAULT_X = 350; | |
| const ACTION_NODE_DEFAULT_Y = 400; | |
| const ACTION_NODE_OFFSET_X = 320; | |
| const newNodePosition = { | |
| x: ACTION_NODE_DEFAULT_X + ACTION_NODE_OFFSET_X * nextIndex, | |
| y: ACTION_NODE_DEFAULT_Y, | |
| }; | |
| try { | |
| const result = await api.nodes.create({ | |
| Name: action.name, | |
| AvailableNodeId: action.id, | |
| Config: { | |
| CredentialsID: "", | |
| }, | |
| WorkflowId: workflowId, | |
| Position: 1, | |
| position: newNodePosition, | |
| stage: cuont, | |
| }); | |
| // Calculate next available action node index (excluding placeholders) | |
| const currentActionNodes = nodes.filter( | |
| (n) => n.data.nodeType === "action" && !n.data.isPlaceholder | |
| ); | |
| const nextIndex = currentActionNodes.length; | |
| // Place new action node after last action (or at default offset if none) | |
| const ACTION_NODE_DEFAULT_X = 350; | |
| const ACTION_NODE_DEFAULT_Y = 400; | |
| const ACTION_NODE_OFFSET_X = 320; | |
| const newNodePosition = { | |
| x: ACTION_NODE_DEFAULT_X + ACTION_NODE_OFFSET_X * nextIndex, | |
| y: ACTION_NODE_DEFAULT_Y, | |
| }; | |
| try { | |
| const result = await api.nodes.create({ | |
| Name: action.name, | |
| AvailableNodeId: action.id, | |
| Config: { | |
| CredentialsID: "", | |
| }, | |
| WorkflowId: workflowId, | |
| position: newNodePosition, | |
| stage: 0, | |
| }); |
🤖 Prompt for AI Agents
In `@apps/web/app/workflows/`[id]/page.tsx around lines 252 - 278, The local
variable `cuont` is a typo and always 0; remove the `let cuont = 0;` declaration
and pass a meaningful stage value to api.nodes.create (replace `stage: cuont`),
e.g. use the computed `nextIndex` (or another explicit constant like 0 if stage
must always be zero); update the call to api.nodes.create to use `stage:
nextIndex` (or `stage: 0`) and delete the unused `cuont` symbol to fix the typo
and remove the unused variable.
| useEffect(() => { | ||
| const fetchWorkflows = async () => { | ||
| try { | ||
| setLoading(true); | ||
| setError(null); | ||
| const response = await api.user.get(); | ||
| const workflows = response.data.Data || response.data || []; | ||
| setData(Array.isArray(workflows) ? workflows : []); | ||
| console.log("The workflow data is", workflows); | ||
| } catch (error) { | ||
| setError("Failed to fetch workflows"); | ||
| console.error("Failed to fetch workflows:", error); | ||
| } finally { | ||
| setLoading(false); | ||
| } | ||
| }; | ||
| if (userId) { | ||
| fetchWorkflows(); | ||
| } | ||
| }, [userId]); |
There was a problem hiding this comment.
Consider handling the case when userId is initially null/undefined.
The effect only fetches when userId is truthy, but loading starts as true. If userId remains falsy (e.g., user not logged in), the UI will show "Loading..." indefinitely.
🐛 Suggested fix
useEffect(() => {
+ if (!userId) {
+ setLoading(false);
+ return;
+ }
const fetchWorkflows = async () => {
try {
setLoading(true);
setError(null);
const response = await api.user.get();
const workflows = response.data.Data || response.data || [];
setData(Array.isArray(workflows) ? workflows : []);
- console.log("The workflow data is", workflows);
} catch (error) {
setError("Failed to fetch workflows");
console.error("Failed to fetch workflows:", error);
} finally {
setLoading(false);
}
};
- if (userId) {
- fetchWorkflows();
- }
+ fetchWorkflows();
}, [userId]);📝 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.
| useEffect(() => { | |
| const fetchWorkflows = async () => { | |
| try { | |
| setLoading(true); | |
| setError(null); | |
| const response = await api.user.get(); | |
| const workflows = response.data.Data || response.data || []; | |
| setData(Array.isArray(workflows) ? workflows : []); | |
| console.log("The workflow data is", workflows); | |
| } catch (error) { | |
| setError("Failed to fetch workflows"); | |
| console.error("Failed to fetch workflows:", error); | |
| } finally { | |
| setLoading(false); | |
| } | |
| }; | |
| if (userId) { | |
| fetchWorkflows(); | |
| } | |
| }, [userId]); | |
| useEffect(() => { | |
| if (!userId) { | |
| setLoading(false); | |
| return; | |
| } | |
| const fetchWorkflows = async () => { | |
| try { | |
| setLoading(true); | |
| setError(null); | |
| const response = await api.user.get(); | |
| const workflows = response.data.Data || response.data || []; | |
| setData(Array.isArray(workflows) ? workflows : []); | |
| } catch (error) { | |
| setError("Failed to fetch workflows"); | |
| console.error("Failed to fetch workflows:", error); | |
| } finally { | |
| setLoading(false); | |
| } | |
| }; | |
| fetchWorkflows(); | |
| }, [userId]); |
🤖 Prompt for AI Agents
In `@apps/web/app/workflows/page.tsx` around lines 25 - 44, The effect currently
only calls fetchWorkflows when userId is truthy, but loading is left true when
userId is falsy; update the useEffect that contains fetchWorkflows so that when
userId is null/undefined you immediately call setLoading(false) (and optionally
setData([]) or setError(null)) instead of leaving the spinner active; change the
logic around useEffect/userId to explicitly handle the else branch and ensure
fetchWorkflows still manages loading/error when invoked.
| Email Input : | ||
|
|
There was a problem hiding this comment.
Incomplete documentation file.
This file was committed with only a label "Email Input :" but no actual JSON structure documentation. Either add the intended JSON structure examples or remove this placeholder file to avoid confusion.
🤖 Prompt for AI Agents
In `@JSON` STRUCTURES.md around lines 1 - 2, The file contains only the
placeholder label "Email Input :" and lacks the intended JSON schema or
examples; either populate it with the concrete JSON structure(s) for the
expected "Email Input" (include required fields, types, example values, and
optional fields) or delete the file to avoid confusion — update the document by
replacing the placeholder label with the complete JSON structure and at least
one example payload so reviewers and consumers can rely on it.
There was a problem hiding this comment.
Pull request overview
This PR implements a workflow model feature that adds a user workflows page, improves workflow canvas with better error handling and positioning logic, adds webhook trigger support, and includes a stage property for workflow nodes.
Changes:
- Added webhook trigger registration to the node registry
- Created a new workflows listing page with card-based UI
- Refactored workflow canvas with defensive position handling and comprehensive error messages
- Added stage property to workflow nodes for execution ordering
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/nodes/src/registry/node-registry.ts | Adds webhook trigger registration during node registry initialization |
| apps/worker/src/tests/setup-test.ts | Adds stage property to test workflow node setup |
| apps/web/app/workflows/page.tsx | New page component for listing user workflows with card-based UI |
| apps/web/app/workflows/[id]/page.tsx | Major refactor with defensive position handling, improved error handling, and workflow state management |
| apps/web/app/workflows/[id]/components/ConfigModal.tsx | Minor formatting change (added commented debug line) |
| apps/web/app/lib/api.ts | Adds new API endpoint for fetching user workflows |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| }, | ||
| WorkflowId: workflowId, | ||
| Position: 1, | ||
| position: newNodePosition, |
There was a problem hiding this comment.
The property name position (lowercase) is inconsistent with other similar API calls in the codebase which use Position (capital P). For example, in the existing code at apps/web/app/workflow/lib/config.ts line 234, the property is sent as Position:. This inconsistency could lead to issues if the backend expects a specific casing.
| position: newNodePosition, | |
| Position: newNodePosition, |
| const Trigger = workflows?.data?.Data?.Trigger; | ||
|
|
||
| if (!Trigger) { | ||
| setError("No trigger found in workflow data, so start with selecting the trigger for the workflow"); |
There was a problem hiding this comment.
The error message is grammatically awkward. It should be rephrased to: "No trigger found in workflow data. Please select a trigger for the workflow." This makes it clearer and more professional.
| setError("No trigger found in workflow data, so start with selecting the trigger for the workflow"); | |
| setError("No trigger found in workflow data. Please select a trigger for the workflow."); |
|
|
||
| // Place the action placeholder after the new node, prevent overlap | ||
| const actionPlaceholderPosition = { | ||
| x: ACTION_NODE_DEFAULT_X + ACTION_NODE_OFFSET_X * (nextIndex + 1) + 400, |
There was a problem hiding this comment.
The placeholder position calculation adds an extra 400 pixels on top of the offset calculation, which seems arbitrary and could cause significant gaps between nodes. The formula ACTION_NODE_DEFAULT_X + ACTION_NODE_OFFSET_X * (nextIndex + 1) + 400 creates a large jump. Consider using a consistent spacing pattern like ACTION_NODE_DEFAULT_X + ACTION_NODE_OFFSET_X * (nextIndex + 1) without the additional 400.
| x: ACTION_NODE_DEFAULT_X + ACTION_NODE_OFFSET_X * (nextIndex + 1) + 400, | |
| x: ACTION_NODE_DEFAULT_X + ACTION_NODE_OFFSET_X * (nextIndex + 1), |
| workflowId: workflow.id, | ||
| AvailableNodeID: availableNode.id, | ||
| position: 0, | ||
| stage : 1, |
There was a problem hiding this comment.
Spacing inconsistency before colon. The property should be stage: 1, not stage : 1, to maintain consistent formatting with the rest of the codebase.
| stage : 1, | |
| stage: 1, |
| return; | ||
| } | ||
|
|
||
| let cuont = 0; |
There was a problem hiding this comment.
The variable name 'cuont' appears to be a typo for 'count'. Additionally, this variable is initialized to 0 but never modified or used in any meaningful way before being passed as the stage parameter. Consider either removing it if not needed or implementing the proper counting logic.
| let cuont = 0; |
| workflowId: workflowId, | ||
| nodes : nodes, | ||
| edges: edges | ||
| edges: edges, |
There was a problem hiding this comment.
The handleSave function only sends edges to the backend but does not send nodes. This appears to be incomplete since the workflow state includes both nodes and edges that should be persisted. Consider whether nodes data also needs to be included in the save operation, or if this is intentional because nodes are saved individually via other API calls.
| edges: edges, | |
| edges: edges, | |
| nodes: nodes, |
| WorkflowId: workflowId, | ||
| Position: 1, | ||
| position: newNodePosition, | ||
| stage: cuont, |
There was a problem hiding this comment.
The variable cuont is set to 0 and never modified, so it always sends stage: 0 for all new action nodes. This likely should increment for each action node added to the workflow to maintain proper stage ordering. Consider implementing logic like const stage = currentActionNodes.length; to properly track the stage sequence.
| import { api } from "../lib/api"; | ||
| import { | ||
| Card, | ||
| CardAction, |
There was a problem hiding this comment.
CardAction is imported but never used in this component. Consider removing this unused import to keep the code clean.
| CardAction, |
| const Trigger = workflows?.data?.Data?.Trigger; | ||
|
|
||
| if (!Trigger) { | ||
| setError("No trigger found in workflow data, so start with selecting the trigger for the workflow"); |
There was a problem hiding this comment.
When no trigger is found and an error is set, the function returns early without calling setError(null) for the success path or managing the error state properly. However, this is actually handled correctly since setError is called with a message. The concern is that this early return occurs before nodes/edges are set, which could leave the UI in a confusing state if a workflow previously had data but now doesn't have a trigger. Consider whether you should clear the nodes and edges state when this error occurs, or ensure the UI properly reflects this error state.
| setError("No trigger found in workflow data, so start with selecting the trigger for the workflow"); | |
| setError( | |
| "No trigger found in workflow data, so start with selecting the trigger for the workflow" | |
| ); | |
| // Clear existing nodes/edges and show the placeholder trigger node | |
| setNodes([ | |
| { | |
| id: "trigger-placeholder", | |
| type: "customNode", | |
| position: DEFAULT_TRIGGER_POSITION, | |
| data: { | |
| label: "Add Trigger", | |
| icon: "➕", | |
| isPlaceholder: true, | |
| nodeType: "trigger", | |
| onConfigure: () => setTriggerOpen(true), | |
| }, | |
| }, | |
| ]); | |
| setEdges([]); |
Summary by CodeRabbit
New Features
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.