feat: add visual workflow builder MVP for automation pipelines (#447)#456
feat: add visual workflow builder MVP for automation pipelines (#447)#456BekkamMallishwari wants to merge 2 commits into
Conversation
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a Visual Workflow Builder with backend workflow storage, CRUD and simulation APIs, a drag-and-drop frontend editor with node configuration and simulation controls, plus integration tests and dashboard navigation updates. ChangesVisual Workflow Builder
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
actor User
participant WorkflowBuilderPage
participant BackendAPI
participant DiskPersistence
User->>WorkflowBuilderPage: Open /workflow-builder
WorkflowBuilderPage->>BackendAPI: GET /api/workflows
BackendAPI->>DiskPersistence: load_workflows_from_disk()
DiskPersistence-->>BackendAPI: workflow list
BackendAPI-->>WorkflowBuilderPage: workflows
User->>WorkflowBuilderPage: Save workflow
WorkflowBuilderPage->>BackendAPI: POST /api/workflows
BackendAPI->>DiskPersistence: save_workflows_to_disk()
BackendAPI-->>WorkflowBuilderPage: saved response
User->>WorkflowBuilderPage: Run simulation
WorkflowBuilderPage->>BackendAPI: POST /api/workflows/{workflow_id}/run
BackendAPI->>DiskPersistence: load_workflows_from_disk()
BackendAPI->>BackendAPI: Simulate queued node steps
BackendAPI-->>WorkflowBuilderPage: status, steps, totals
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ 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 |
|
Hi @sreerevanth This implements the MVP for the Visual Workflow Builder for Issue #447. All checks are passing and it’s ready for review whenever convenient. Thanks! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (5)
frontend/pages/index.tsx (1)
334-337: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winUse Next.js
<Link>component instead of plain<a>for client-side navigation.The link to
/workflow-builderuses a standard HTML<a>tag, which causes a full page reload in a Next.js SPA. The<Link>component provides efficient client-side navigation without reloads.import Link from 'next/link'Then update the link:
- <a href="/workflow-builder" className="inline-flex items-center gap-2 rounded-xl border border-blue-500/30 bg-blue-500/10 px-4 py-2 text-sm font-medium text-blue-300 transition hover:bg-blue-500/20 hover:text-blue-200"> + <Link href="/workflow-builder" className="inline-flex items-center gap-2 rounded-xl border border-blue-500/30 bg-blue-500/10 px-4 py-2 text-sm font-medium text-blue-300 transition hover:bg-blue-500/20 hover:text-blue-200"> <Zap size={14} /> Workflow Builder - </a> + </Link>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/pages/index.tsx` around lines 334 - 337, The navigation link to "/workflow-builder" uses a plain HTML `<a>` tag instead of Next.js `<Link>` component, which causes unnecessary full page reloads. Replace the `<a>` tag with the `<Link>` component imported from 'next/link' at the top of the file, and move the className and other styling attributes to the appropriate elements within the Link component structure. This ensures efficient client-side navigation without page reloads in the Next.js SPA.frontend/.eslintrc.json (1)
1-8: 🧹 Nitpick | 🔵 Trivial | ⚖️ Poor tradeoffConsider scoping ESLint rule disables to specific locations rather than disabling at the project level.
Disabling
react-hooks/exhaustive-deps,@next/next/no-html-link-for-pages, andreact/no-unescaped-entitiesat the.eslintrc.jsonroot applies these exceptions to the entire frontend codebase. This masks potential issues beyond the workflow builder:
react-hooks/exhaustive-deps: Disabling this rule project-wide risks stale-closure bugs in any useEffect across the frontend. The pattern inworkflow-builder.tsx(lines 135–140 in that file) may have a valid reason to omitselectedWorkflowIdfrom deps, but this intent should be documented with an inline ESLint comment at that specific location, not hidden by a project-wide disable.
@next/next/no-html-link-for-pages: This combined with the<a>tag in index.tsx suggests a pattern-wide choice to avoid<Link>, which is suboptimal for SPA navigation.
react/no-unescaped-entities: Unclear why this needs to be disabled project-wide unless multiple files contain unescaped entities.Recommended approach: Move targeted disables (e.g.,
// eslint-disable-next-line react-hooks/exhaustive-deps) to the specific lines inworkflow-builder.tsxthat require exceptions, with explanatory comments. Keep the root.eslintrc.jsonstrict to catch issues elsewhere.Please verify the scope and necessity of these disables by checking:
- Does this
.eslintrc.jsonapply to all frontend pages or only the workflow-builder?- Which patterns in the workflow-builder necessitate each disable, and are there equivalent patterns elsewhere in the frontend?
- Is the
selectedWorkflowIdomission from the useEffect dependency array in workflow-builder.tsx intentional, and does theloadWorkflowfunction safely handle the closure?🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/.eslintrc.json` around lines 1 - 8, Remove the three rules `@next/next/no-html-link-for-pages`, `react/no-unescaped-entities`, and `react-hooks/exhaustive-deps` from the rules section in the root .eslintrc.json file. Then, for each rule that is truly necessary to disable, locate the specific files and lines where exceptions are needed (particularly in workflow-builder.tsx around the useEffect block with the selectedWorkflowId dependency array omission) and add targeted inline ESLint disable comments using the format `// eslint-disable-next-line rule-name` directly above those lines, followed by explanatory comments documenting why the exception is necessary at that specific location. Only re-add rules to .eslintrc.json if they are genuinely needed across multiple unrelated parts of the codebase; otherwise, keep exceptions scoped to their specific locations.tests/test_workflows.py (1)
15-63: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winAdd coverage for
/api/v1/workflowsalias routes to protect compatibility guarantees.This lifecycle test only exercises
/api/workflows, but the backend exposes/api/v1/workflowsaliases as part of the contract. A small parametrization over base path would prevent silent regressions.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_workflows.py` around lines 15 - 63, The test_workflows_lifecycle function only exercises the /api/workflows endpoint path, but the backend also provides /api/v1/workflows as an alias route that needs coverage. Add a pytest parametrize decorator to the test_workflows_lifecycle function to accept a base_path parameter with two values: "/api/workflows" and "/api/v1/workflows". Then replace all hardcoded endpoint paths in the test (such as "/api/workflows", "/api/workflows/wf-test-1234") with dynamically constructed paths using the base_path parameter to ensure both alias routes are tested.frontend/pages/workflow-builder.tsx (2)
834-840: 🧹 Nitpick | 🔵 Trivial | ⚖️ Poor tradeoffMinimap uses hardcoded canvas dimensions that may not match actual content.
The minimap positions nodes based on fixed 1400×800 dimensions. Nodes positioned beyond these coordinates will be clamped to the minimap edges, potentially showing inaccurate positions for large workflows.
Consider computing bounds dynamically from actual node positions, or documenting the assumed canvas size.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/pages/workflow-builder.tsx` around lines 834 - 840, The minimap style calculation uses hardcoded canvas dimensions (1400 for width and 800 for height) when computing node positions, which causes nodes to be clamped inaccurately for workflows larger than these dimensions. Replace the hardcoded 1400 and 800 values in the left and top style calculations with dynamically computed bounds. Calculate these bounds by finding the maximum x and y coordinates across all nodes in the workflow, or retrieve the actual canvas dimensions from the component state or props if available. This ensures the minimap accurately represents node positions regardless of workflow size.
180-180: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueReplace deprecated
substr()withsubstring()orslice().
String.prototype.substr()is deprecated. The patternMath.random().toString(36).substr(2, 9)appears in multiple places for ID generation.Proposed fix using substring
-const newId = `wf-${Math.random().toString(36).substr(2, 9)}` +const newId = `wf-${Math.random().toString(36).substring(2, 11)}`Apply similar changes at lines 197, 327, and 355.
Also applies to: 197-197, 327-327, 355-355
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/pages/workflow-builder.tsx` at line 180, The code uses the deprecated String.prototype.substr() method in the ID generation pattern. Replace all occurrences of .substr(2, 9) with .substring(2, 11) or .slice(2, 11) to use the non-deprecated alternatives. This needs to be applied in four locations: the newId variable assignment and at the three other locations where this same pattern appears for ID generation (referenced at lines 197, 327, and 355). Remember that substring and slice use start and end indices, not start and length like substr does, so adjust accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@agentwatch/api/server.py`:
- Around line 1276-1297: The save_workflow function has a race condition where
concurrent requests can lose updates because the load-modify-save sequence is
not atomic. Multiple requests can read the workflows file simultaneously, each
modify it independently, and the last write will overwrite previous changes.
Wrap the entire sequence between load_workflows_from_disk() and
save_workflows_to_disk() with file locking using fcntl.flock to ensure only one
request can modify workflows at a time. Acquire the lock before loading and
release it after saving to make the operation atomic and prevent concurrent
modifications from overwriting each other.
- Around line 1336-1350: The incoming_counts dictionary is initialized based on
edge counts but is never decremented during the execution loop, causing nodes to
execute as soon as they're queued rather than waiting for all dependencies to
complete. After each node is executed (processed from the queue), iterate
through its outgoing edges using the outgoing_edges dictionary, decrement the
incoming_counts for each target node, and add the target to the queue only when
its incoming_counts reaches zero. This ensures all predecessors complete before
a node executes, implementing proper DAG traversal semantics.
- Around line 1253-1259: The save_workflows_to_disk function silently catches
exceptions and only logs a warning, but the calling save_workflow endpoint
proceeds to return a success status regardless of whether the write actually
succeeded. To fix this, modify save_workflows_to_disk to either re-raise the
caught exception after logging or return a boolean indicating success/failure.
Then update the save_workflow endpoint to check the result from
save_workflows_to_disk before returning the success response to the caller,
ensuring the user is only told their workflow was saved when it actually was
persisted.
In `@frontend/pages/workflow-builder.tsx`:
- Around line 226-243: In the handleDeleteWorkflow function, when the currently
selected workflow is deleted (when selectedWorkflowId === id), only the
selectedWorkflowId is being cleared to null. To prevent stale canvas data from
remaining visible, also clear the nodes and edges state at the same location by
calling the appropriate state setters (likely setNodes and setEdges) to reset
them to empty arrays alongside the setSelectedWorkflowId(null) call.
- Line 38: The fetcher function is returning null for non-ok HTTP responses,
which prevents SWR from detecting and handling errors properly. Instead of
returning null when r.ok is false, throw an error that includes the response
status and details so that SWR's error state and retry logic can be properly
triggered for 4xx and 5xx responses.
- Around line 640-642: The instructional text "Mouse wheel to zoom" in the div
element at line 640-642 advertises functionality that isn't implemented. To fix
this, implement a handleWheel event handler function that prevents default
behavior and adjusts the zoom state by calculating a delta based on the wheel
direction (negative deltaY increases zoom, positive decreases it), clamping the
result between 0.5 and 1.8 using the setZoom function. Then add the onWheel
event listener with this handler to the main canvas element to actually enable
the wheel zoom functionality.
- Around line 210-223: The fetch request to the workflows API endpoint is
missing the required X-Api-Key header. First, determine where the API key is
stored or accessed on the frontend (check context, local storage, or component
props). Then, in the fetch call that uses `${API_BASE}/workflows`, add the
X-Api-Key header to the headers object alongside the existing Content-Type
header, passing the API key value. Apply this same header addition to all other
workflow API calls mentioned for delete and list operations to ensure all
requests include proper authentication and avoid 401 errors.
In `@tests/test_workflows.py`:
- Around line 10-13: The client fixture is using the real app without isolating
disk state, causing tests to mutate shared files and creating test dependencies.
Modify the client fixture to override the WORKFLOWS_FILE environment variable to
use a temporary or isolated test-specific file path before creating the
TestClient. Additionally, ensure the fixture properly cleans up the temporary
file after the test completes using a yield statement or finally block to
guarantee cleanup even when tests fail. Apply the same isolation pattern to
other test functions referenced at lines 23-25 and 57-63 that also interact with
the workflows file.
---
Nitpick comments:
In `@frontend/.eslintrc.json`:
- Around line 1-8: Remove the three rules `@next/next/no-html-link-for-pages`,
`react/no-unescaped-entities`, and `react-hooks/exhaustive-deps` from the rules
section in the root .eslintrc.json file. Then, for each rule that is truly
necessary to disable, locate the specific files and lines where exceptions are
needed (particularly in workflow-builder.tsx around the useEffect block with the
selectedWorkflowId dependency array omission) and add targeted inline ESLint
disable comments using the format `// eslint-disable-next-line rule-name`
directly above those lines, followed by explanatory comments documenting why the
exception is necessary at that specific location. Only re-add rules to
.eslintrc.json if they are genuinely needed across multiple unrelated parts of
the codebase; otherwise, keep exceptions scoped to their specific locations.
In `@frontend/pages/index.tsx`:
- Around line 334-337: The navigation link to "/workflow-builder" uses a plain
HTML `<a>` tag instead of Next.js `<Link>` component, which causes unnecessary
full page reloads. Replace the `<a>` tag with the `<Link>` component imported
from 'next/link' at the top of the file, and move the className and other
styling attributes to the appropriate elements within the Link component
structure. This ensures efficient client-side navigation without page reloads in
the Next.js SPA.
In `@frontend/pages/workflow-builder.tsx`:
- Around line 834-840: The minimap style calculation uses hardcoded canvas
dimensions (1400 for width and 800 for height) when computing node positions,
which causes nodes to be clamped inaccurately for workflows larger than these
dimensions. Replace the hardcoded 1400 and 800 values in the left and top style
calculations with dynamically computed bounds. Calculate these bounds by finding
the maximum x and y coordinates across all nodes in the workflow, or retrieve
the actual canvas dimensions from the component state or props if available.
This ensures the minimap accurately represents node positions regardless of
workflow size.
- Line 180: The code uses the deprecated String.prototype.substr() method in the
ID generation pattern. Replace all occurrences of .substr(2, 9) with
.substring(2, 11) or .slice(2, 11) to use the non-deprecated alternatives. This
needs to be applied in four locations: the newId variable assignment and at the
three other locations where this same pattern appears for ID generation
(referenced at lines 197, 327, and 355). Remember that substring and slice use
start and end indices, not start and length like substr does, so adjust
accordingly.
In `@tests/test_workflows.py`:
- Around line 15-63: The test_workflows_lifecycle function only exercises the
/api/workflows endpoint path, but the backend also provides /api/v1/workflows as
an alias route that needs coverage. Add a pytest parametrize decorator to the
test_workflows_lifecycle function to accept a base_path parameter with two
values: "/api/workflows" and "/api/v1/workflows". Then replace all hardcoded
endpoint paths in the test (such as "/api/workflows",
"/api/workflows/wf-test-1234") with dynamically constructed paths using the
base_path parameter to ensure both alias routes are tested.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 94767b74-1459-4b6f-befe-66838dcea61e
📒 Files selected for processing (5)
agentwatch/api/server.pyfrontend/.eslintrc.jsonfrontend/pages/index.tsxfrontend/pages/workflow-builder.tsxtests/test_workflows.py
🧪 PR Test Results
Python 3.12 · commit 74a56bb |
|
@BekkamMallishwari can u please check the ci |
|
@BekkamMallishwari |
|
@BekkamMallishwari Thanks for the impressive work! Before I merge, could you explain why this dashboard feature requires changes to security/, rollback/, mcp_server, and auth? I'd just like to understand the architectural reasoning behind those modifications since they're outside the original scope of the issue. |
|
Hi @sreerevanth , The changes in The actual implementation for Issue #447 is limited to the workflow builder UI, its supporting API endpoints, routing, and tests. I'll clean up the branch to remove any unrelated changes so the PR only contains the files relevant to this feature. Thanks for pointing this out! |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@frontend/pages/index.tsx`:
- Around line 296-298: The DashboardPage refresh logic is referencing missing
bindings, so define a local refresh helper before it is used and reuse it for
both the socket callback and the button handler. Update the handler in
DashboardPage to replace the nonexistent refreshSummary()/refreshSessions()
calls with the shared helper, and make sure that helper also invalidates the
blocked-events cache because SafetyPanel now depends on useBlockedEvents().
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 7a3cae07-9674-4f59-93f9-6bf1140495ac
📒 Files selected for processing (2)
agentwatch/api/server.pyfrontend/pages/index.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- agentwatch/api/server.py
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@frontend/pages/index.tsx`:
- Around line 296-298: The DashboardPage refresh logic is referencing missing
bindings, so define a local refresh helper before it is used and reuse it for
both the socket callback and the button handler. Update the handler in
DashboardPage to replace the nonexistent refreshSummary()/refreshSessions()
calls with the shared helper, and make sure that helper also invalidates the
blocked-events cache because SafetyPanel now depends on useBlockedEvents().
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 7a3cae07-9674-4f59-93f9-6bf1140495ac
📒 Files selected for processing (2)
agentwatch/api/server.pyfrontend/pages/index.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- agentwatch/api/server.py
🛑 Comments failed to post (1)
frontend/pages/index.tsx (1)
296-298: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Define the refresh handler before using it.
Line 323 calls
refreshSummary()andrefreshSessions(), but neither binding exists inDashboardPage, so this will fail type-check/build. Reuse one local refresh helper for the socket callback and button; include the blocked-events cache sinceSafetyPanelnow depends onuseBlockedEvents().Suggested fix
const { summary, isSummaryLoading: summaryLoading } = useDashboardSummary() const { sessions, isSessionsLoading } = useSessions() const { blockedEvents, isBlockedLoading } = useBlockedEvents() const [liveEvents, setLiveEvents] = useState<AgentEvent[]>([]) + const refreshDashboardData = () => { + void queryClient.invalidateQueries({ queryKey: ['dashboard'] }) + void queryClient.invalidateQueries({ queryKey: ['sessions'] }) + void queryClient.invalidateQueries({ queryKey: ['safety', 'blocked'] }) + } const { status: wsStatus, reconnectElapsedSec } = useLiveEventSocket( (event) => { setLiveEvents((previous) => [event, ...previous].slice(0, 200)) }, - () => { - queryClient.invalidateQueries({ queryKey: ['dashboard'] }) - queryClient.invalidateQueries({ queryKey: ['sessions'] }) - }, + refreshDashboardData, )- <button onClick={() => { refreshSummary(); refreshSessions() }} aria-label="Refresh dashboard data" className="inline-flex items-center gap-2 rounded-xl border border-white/10 bg-white/5 px-4 py-2 text-sm text-zinc-300 transition hover:bg-white/10 hover:text-white"> + <button onClick={refreshDashboardData} aria-label="Refresh dashboard data" className="inline-flex items-center gap-2 rounded-xl border border-white/10 bg-white/5 px-4 py-2 text-sm text-zinc-300 transition hover:bg-white/10 hover:text-white">Also applies to: 323-323
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/pages/index.tsx` around lines 296 - 298, The DashboardPage refresh logic is referencing missing bindings, so define a local refresh helper before it is used and reuse it for both the socket callback and the button handler. Update the handler in DashboardPage to replace the nonexistent refreshSummary()/refreshSessions() calls with the shared helper, and make sure that helper also invalidates the blocked-events cache because SafetyPanel now depends on useBlockedEvents().
|
@BekkamMallishwari please check out all your prs if possible finish them up by tommorow |
2 participants
Summary
Minimal Viable Product (MVP) of the Visual Workflow Builder for Agent Automation Pipelines. Users can visualize, add, move, and connect nodes (Agent, LLM, Memory, HTTP, etc.) on a responsive interactive canvas, and configure node-specific properties. A simulated execution pipeline can be run, showing real-time execution steps and logs.
Changes
/workflow-builderroute, implementing basic node system, drag-and-drop mechanics, interactive connection rendering (SVG-based bezier curves), and viewport navigation (zoom/pan).GET /api/workflows,POST /api/workflows,GET /api/workflows/{id}, andDELETE /api/workflows/{id}, alongside duplicate/alias routes for compatibility./(index page) directly to the visual builder.Testing
tests/test_workflows.pyverifying all REST API endpoints.npm run type-checkpassed.npm run lintpassed.npm run buildpassed.Limitations
Issue
Closes #447
Summary by CodeRabbit