-
Notifications
You must be signed in to change notification settings - Fork 41
fix: ReactFlow controls not working in dashboard graph visualizations #429
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. Weβll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| 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, { | ||||||||||||||||||||||
|
|
@@ -257,6 +257,9 @@ const GraphVisualizer: React.FC<{ nodes: NodeTemplate[] }> = ({ nodes }) => { | |||||||||||||||||||||
| ); | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| const [isInteractive, setIsInteractive] = useState(true); | ||||||||||||||||||||||
| const handleInteractiveChange = useCallback((val: boolean) => setIsInteractive(val), []); | ||||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You can simplify the code by removing this onInteractiveChange={setIsInteractive} |
||||||||||||||||||||||
|
|
||||||||||||||||||||||
|
Comment on lines
+260
to
+262
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. (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. (lint/correctness/useHookAtTopLevel) π€ Prompt for AI Agents |
||||||||||||||||||||||
| return ( | ||||||||||||||||||||||
| <Card> | ||||||||||||||||||||||
| <CardHeader className="pb-3"> | ||||||||||||||||||||||
|
|
@@ -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
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||||||||||||||||||||||
| > | ||||||||||||||||||||||
| <Controls /> | ||||||||||||||||||||||
| <Controls | ||||||||||||||||||||||
| position="bottom-left" | ||||||||||||||||||||||
| onInteractiveChange={handleInteractiveChange} | ||||||||||||||||||||||
| /> | ||||||||||||||||||||||
|
Comment on lines
+295
to
+298
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Suggested change
π€ Prompt for AI Agents |
||||||||||||||||||||||
| </ReactFlow> | ||||||||||||||||||||||
| </div> | ||||||||||||||||||||||
| </CardContent> | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -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'; | ||||||||||||||||||||||||||||||
|
|
@@ -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'; | ||||||||||||||||||||||||||||||
|
|
@@ -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); | ||||||||||||||||||||||||||||||
|
|
@@ -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
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||||||||||||||||||||||||||||||
| <Background color="#031035" /> | ||||||||||||||||||||||||||||||
| <Controls /> | ||||||||||||||||||||||||||||||
| <Controls | ||||||||||||||||||||||||||||||
| showInteractive={true} | ||||||||||||||||||||||||||||||
| showFitView={true} | ||||||||||||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
|
||||||||||||||||||||||||||||||
| 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
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Suggested change
π€ Prompt for AI Agents |
||||||||||||||||||||||||||||||
| <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
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This custom panel is a great UX improvement. However, the 'Refresh' button here duplicates an existing 'Refresh' button at the top of the |
||||||||||||||||||||||||||||||
| </ReactFlow> | ||||||||||||||||||||||||||||||
| </div> | ||||||||||||||||||||||||||||||
| </CardContent> | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The
ReactFlowProvideris correctly added forGraphVisualization, but theGraphTemplateDetailcomponent, 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
ReactFlowProviderat a higher level, wrapping all components that need the React Flow context. I'd suggest moving the provider to wrap the top-leveldivin this component'sreturnstatement (at line 75). This would provide the context to bothGraphVisualizationandGraphTemplateDetail.Example:
With that change, you can remove the
ReactFlowProviderwrapper from aroundGraphVisualizationhere.