Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 8 additions & 5 deletions dashboard/src/app/graph/[namespace]/[runId]/page.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

import React, { useState, useCallback } from 'react';
import { useParams, useRouter } from 'next/navigation';
import { ReactFlowProvider } from 'reactflow';
import { GraphVisualization } from '@/components/GraphVisualization';
import { GraphTemplateDetail } from '@/components/GraphTemplateDetail';
import { ThemeToggle } from '@/components/ThemeToggle';
Expand Down Expand Up @@ -103,11 +104,13 @@ export default function GraphPage() {

{/* Main Content */}
<main className="max-w-7xl mx-auto px-4 sm:px-6 lg:px-8 py-8">
<GraphVisualization
namespace={namespace}
runId={runId}
onGraphTemplateRequest={handleOpenGraphTemplate}
/>
<ReactFlowProvider>
<GraphVisualization
namespace={namespace}
runId={runId}
onGraphTemplateRequest={handleOpenGraphTemplate}
/>
</ReactFlowProvider>
Comment on lines +107 to +113
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

The ReactFlowProvider is correctly added for GraphVisualization, but the GraphTemplateDetail component, which also uses React Flow controls, is not wrapped by a provider. This will likely cause its controls to not function as expected.

A better approach would be to have a single ReactFlowProvider at a higher level, wrapping all components that need the React Flow context. I'd suggest moving the provider to wrap the top-level div in this component's return statement (at line 75). This would provide the context to both GraphVisualization and GraphTemplateDetail.

Example:

// at line 74
return (
  <ReactFlowProvider>
    <div className="min-h-screen bg-background">
      {/* ... rest of the component ... */}
    </div>
  </ReactFlowProvider>
);

With that change, you can remove the ReactFlowProvider wrapper from around GraphVisualization here.

</main>

{/* Graph Template Detail Modal - Inline at bottom */}
Expand Down
19 changes: 15 additions & 4 deletions dashboard/src/components/GraphTemplateDetail.tsx
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
'use client';

import React, { useMemo } from 'react';
import React, { useMemo, useState, useCallback } from 'react';
import { UpsertGraphTemplateResponse, NodeTemplate } from '@/types/state-manager';
import { X, GitBranch, Settings, Database, Workflow, Clock } from 'lucide-react';
import ReactFlow, {
Expand Down Expand Up @@ -257,6 +257,9 @@ const GraphVisualizer: React.FC<{ nodes: NodeTemplate[] }> = ({ nodes }) => {
);
}

const [isInteractive, setIsInteractive] = useState(true);
const handleInteractiveChange = useCallback((val: boolean) => setIsInteractive(val), []);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

You can simplify the code by removing this useCallback. The state setter function from useState (setIsInteractive) is guaranteed by React to be stable and doesn't need to be memoized. You can pass it directly to the onInteractiveChange prop in the <Controls> component at line 297:

onInteractiveChange={setIsInteractive}


Comment on lines +260 to +262
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | πŸ”΄ Critical

Fix rules-of-hooks violation (hooks after early return).

useState/useCallback are declared after a conditional return (Line 240), which will crash with "Rendered fewer hooks than expected" when nodes length changes.

Apply this diff to remove the hooks here:

-  const [isInteractive, setIsInteractive] = useState(true);
-  const handleInteractiveChange = useCallback((val: boolean) => setIsInteractive(val), []);

Then add them immediately after the useEdgesState declaration (before the early return), e.g.:

// place right after:
const [flowEdgesState, , onEdgesChange] = useEdgesState(flowEdges);

const [isInteractive, setIsInteractive] = useState(true);
const handleInteractiveChange = useCallback((val: boolean) => setIsInteractive(val), []);
🧰 Tools
πŸͺ› Biome (2.1.2)

[error] 260-260: This hook is being called conditionally, but all hooks must be called in the exact same order in every component render.

Hooks should not be called after an early return.

For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level

(lint/correctness/useHookAtTopLevel)


[error] 261-261: This hook is being called conditionally, but all hooks must be called in the exact same order in every component render.

Hooks should not be called after an early return.

For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level

(lint/correctness/useHookAtTopLevel)

πŸ€– Prompt for AI Agents
In dashboard/src/components/GraphTemplateDetail.tsx around lines 240-262, the
useState/useCallback hooks for isInteractive are currently declared after an
early conditional return which violates the Rules of Hooks; remove the existing
declarations at lines ~260-262 and instead add them immediately after the
useEdgesState declaration (right after the line where flowEdgesState and
onEdgesChange are declared) so the hooks run unconditionally before any returns,
e.g., move the two hook declarations to just after useEdgesState.

return (
<Card>
<CardHeader className="pb-3">
Expand All @@ -279,12 +282,20 @@ const GraphVisualizer: React.FC<{ nodes: NodeTemplate[] }> = ({ nodes }) => {
minZoom={0.1}
maxZoom={2}
defaultViewport={{ x: 0, y: 0, zoom: 1.5 }}
elementsSelectable={true}
elementsSelectable={isInteractive}
nodesConnectable={false}
nodesDraggable={false}
nodesDraggable={isInteractive}
zoomOnScroll={isInteractive}
panOnScroll={isInteractive}
panOnDrag={isInteractive}
zoomOnPinch={isInteractive}
zoomOnDoubleClick={isInteractive}
className="bg-background"
Comment on lines +285 to 293
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

nodesDraggable toggle is ineffective due to per-node draggable=false.

Each node is created with draggable: false (Lines 193–194), which overrides the global nodesDraggable={isInteractive} and prevents dragging even in interactive mode.

Fix: remove the per-node draggable flag from node creation so the global toggle works:

// In flowNodes.push(...) remove this line:
draggable: false,

Optional: if you want the graph to reflect updated interactivity when props change, sync state like in GraphVisualization:

const [flowNodesState, setFlowNodesState, onNodesChange] = useNodesState(flowNodes);
const [flowEdgesState, setFlowEdgesState, onEdgesChange] = useEdgesState(flowEdges);

useEffect(() => {
  setFlowNodesState(flowNodes);
  setFlowEdgesState(flowEdges);
}, [flowNodes, flowEdges, setFlowNodesState, setFlowEdgesState]);
πŸ€– Prompt for AI Agents
In dashboard/src/components/GraphTemplateDetail.tsx around lines 285 to 293,
nodes are still created with a per-node draggable: false which overrides the
global nodesDraggable={isInteractive} and prevents dragging; remove the
draggable: false property from each node in the flowNodes creation so the global
nodesDraggable prop takes effect, and optionally, if you need the nodes/edges to
reflect prop changes, initialize and sync state using
useNodesState/useEdgesState and a useEffect to set the flow state when
flowNodes/flowEdges change.

>
<Controls />
<Controls
position="bottom-left"
onInteractiveChange={handleInteractiveChange}
/>
Comment on lines +295 to +298
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick | πŸ”΅ Trivial

Explicitly show interactive/fit buttons for Controls (consistency).

Make Controls behavior explicit and consistent with GraphVisualization.

-            <Controls 
-              position="bottom-left"
-              onInteractiveChange={handleInteractiveChange} 
-            />
+            <Controls 
+              position="bottom-left"
+              showInteractive={true}
+              showFitView={true}
+              onInteractiveChange={handleInteractiveChange} 
+            />
πŸ“ 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
<Controls
position="bottom-left"
onInteractiveChange={handleInteractiveChange}
/>
<Controls
position="bottom-left"
showInteractive={true}
showFitView={true}
onInteractiveChange={handleInteractiveChange}
/>
πŸ€– Prompt for AI Agents
In dashboard/src/components/GraphTemplateDetail.tsx around lines 295 to 298,
Controls is rendered without explicitly enabling the interactive/fit buttons
which makes its behavior inconsistent with GraphVisualization; update the
Controls invocation to pass the same explicit props used by GraphVisualization
(e.g., showInteractiveButton and showFitButton or the specific prop names used
in your Controls API) and set them to true so the interactive and fit controls
are always visible and consistent across components.

</ReactFlow>
</div>
</CardContent>
Expand Down
54 changes: 49 additions & 5 deletions dashboard/src/components/GraphVisualization.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,9 @@ import ReactFlow, {
MarkerType,
NodeTypes,
ConnectionLineType,
Handle
Handle,
useReactFlow,
Panel
} from 'reactflow';
import 'reactflow/dist/style.css';
import { clientApiService } from '@/services/clientApi';
Expand All @@ -29,7 +31,8 @@ import {
XCircle,
Loader2,
Network,
BarChart3
BarChart3,
Maximize2
} from 'lucide-react';
import { Card, CardHeader, CardTitle, CardDescription, CardContent } from '@/components/ui/card';
import { Button } from '@/components/ui/button';
Expand Down Expand Up @@ -124,13 +127,22 @@ export const GraphVisualization: React.FC<GraphVisualizationProps> = ({
runId,
onGraphTemplateRequest
}) => {
const { fitView } = useReactFlow();
const [graphData, setGraphData] = useState<GraphStructureResponse | null>(null);
const [isLoading, setIsLoading] = useState(false);
const [error, setError] = useState<string | null>(null);
const [selectedNode, setSelectedNode] = useState<GraphNodeType | null>(null);
const [selectedNodeDetails, setSelectedNodeDetails] = useState<NodeRunDetailsResponse | null>(null);
const [isLoadingNodeDetails, setIsLoadingNodeDetails] = useState(false);
const [nodeDetailsError, setNodeDetailsError] = useState<string | null>(null);
const [isInteractive, setIsInteractive] = useState(true);

const handleFitView = useCallback(() => {
fitView({
duration: 800, // Animation duration in ms
padding: 0.2 // 20% padding around the graph
});
}, [fitView]);

const loadGraphStructure = useCallback(async () => {
setIsLoading(true);
Expand Down Expand Up @@ -525,12 +537,44 @@ export const GraphVisualization: React.FC<GraphVisualizationProps> = ({
defaultViewport={{ x: 0, y: 0, zoom: 0.7 }}
proOptions={{ hideAttribution: true }}
connectionLineType={ConnectionLineType.Straight}
elementsSelectable={true}
elementsSelectable={isInteractive}
nodesConnectable={false}
nodesDraggable={false}
nodesDraggable={isInteractive}
zoomOnScroll={isInteractive}
panOnScroll={isInteractive}
panOnDrag={isInteractive}
zoomOnPinch={isInteractive}
zoomOnDoubleClick={isInteractive}
>
Comment on lines +540 to 548
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

nodesDraggable toggle is overridden by node.draggable=false.

Nodes are constructed with draggable: false (Line 286), which disables dragging even when isInteractive is true.

Fix: remove the per-node flag so the global nodesDraggable={isInteractive} takes effect:

// In reactFlowNodes.push(...) remove:
draggable: false,
πŸ€– Prompt for AI Agents
In dashboard/src/components/GraphVisualization.tsx around lines 540 to 548 and
node creation at ~line 286, the nodes are being created with draggable: false
which overrides the global nodesDraggable={isInteractive}; remove the per-node
draggable: false property from the object pushed into reactFlowNodes so the
global nodesDraggable prop can control draggability, then run the component to
verify nodes are draggable when isInteractive is true.

<Background color="#031035" />
<Controls />
<Controls
showInteractive={true}
showFitView={true}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Since you've added a custom 'Fit View' button in the top-right panel which provides a better user experience with smooth animation, it's a good idea to hide the default 'Fit View' button from the built-in controls to avoid duplication and provide a cleaner UI.

Suggested change
showFitView={true}
showFitView={false}

position="bottom-left"
onInteractiveChange={setIsInteractive}
/>
<Panel position="top-right" className="flex gap-2 bg-card/90 backdrop-blur-sm rounded-lg p-2 border shadow-lg">
Comment on lines +550 to +556
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick | πŸ”΅ Trivial

Avoid duplicate Fit View actions (optional).

You already provide a β€œFit View” button in the Panel. Consider disabling the Control’s Fit View to reduce UI duplication.

-              <Controls 
-                showInteractive={true}
-                showFitView={true}
+              <Controls 
+                showInteractive={true}
+                showFitView={false}
                 position="bottom-left"
                 onInteractiveChange={setIsInteractive}
               />
πŸ“ 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
<Controls
showInteractive={true}
showFitView={true}
position="bottom-left"
onInteractiveChange={setIsInteractive}
/>
<Panel position="top-right" className="flex gap-2 bg-card/90 backdrop-blur-sm rounded-lg p-2 border shadow-lg">
<Controls
showInteractive={true}
showFitView={false}
position="bottom-left"
onInteractiveChange={setIsInteractive}
/>
<Panel position="top-right" className="flex gap-2 bg-card/90 backdrop-blur-sm rounded-lg p-2 border shadow-lg">
πŸ€– Prompt for AI Agents
In dashboard/src/components/GraphVisualization.tsx around lines 550 to 556, the
Controls component renders a duplicate "Fit View" action that is also provided
in the Panel; remove or disable the duplicate by changing the Controls props to
not show the Fit View (e.g., set showFitView={false} or remove the prop) so only
the Panel contains the Fit View button, keeping onInteractiveChange and other
props unchanged.

<Button
size="sm"
variant="outline"
onClick={handleFitView}
className="text-xs h-8"
title="Fit graph to screen"
>
<Maximize2 className="w-3 h-3 mr-1" />
Fit View
</Button>
<Button
size="sm"
variant="outline"
onClick={loadGraphStructure}
className="text-xs h-8"
title="Refresh graph data"
>
<RefreshCw className="w-3 h-3 mr-1" />
Refresh
</Button>
</Panel>
Comment on lines +556 to +577
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

This custom panel is a great UX improvement. However, the 'Refresh' button here duplicates an existing 'Refresh' button at the top of the GraphVisualization component (line 394). Both buttons call the same loadGraphStructure function. To avoid user confusion from redundant controls, it would be best to have only one. Since this new one inside the panel is contextually better placed, I'd recommend removing the older one from the component's header.

</ReactFlow>
</div>
</CardContent>
Expand Down