GENIE-1218/story/Implement graph view using a separate library#78
GENIE-1218/story/Implement graph view using a separate library#78
Conversation
…ty-ai-tools/UnifAI into graph-display-new-library
…to graph-display-new-library
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughReplaces the ReactFlow-based editor with a JointJS-driven GraphDisplay and useJointGraph hook, centralizes resolved-blueprint fetching and caching with cancellation guards, adds theme-derived color utilities and widespread theming, removes ReactFlowGraph, and updates consumers and UI overlays to integrate the new graph system. Changes
Sequence Diagram(s)sequenceDiagram
participant Component as Agentic Component
participant GraphDisplay as GraphDisplay
participant useJointGraph as useJointGraph Hook
participant JointJS as JointJS Engine
participant Streaming as StreamingDataContext
participant ValidationModal as ValidationResultModal
Component->>GraphDisplay: render(blueprintId, specDict, props)
GraphDisplay->>useJointGraph: init(options)
useJointGraph->>JointJS: create Graph & Paper, inject SVG defs
useJointGraph->>useJointGraph: graphFlowToLayoutData(specDict)
JointJS->>JointJS: layout nodes/edges, apply markers/animations
useJointGraph->>GraphDisplay: expose overlays, paperTransform
GraphDisplay->>Streaming: subscribe node status (if live)
Streaming-->>GraphDisplay: node status updates
GraphDisplay->>GraphDisplay: update overlays & visuals
Component->>GraphDisplay: user interacts (click badge/validation)
GraphDisplay->>ValidationModal: open with node validation details
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (2 warnings)
✅ 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.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
…at-community-ai-tools/UnifAI into graph-display-new-library
…at-community-ai-tools/UnifAI into graph-display-new-library
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ui/client/src/components/agentic-ai/WorkflowsPanel.tsx (1)
339-367:⚠️ Potential issue | 🟡 Minor
GraphDisplaywithheight="100%"may not fill remaining flex space.The parent div (Line 339) is a
flex flex-colcontainer. The Share Section takes fixed height, and GraphDisplay needs to fill the rest. A child withheight: 100%in a flex column may not expand as expected — consider wrapping GraphDisplay in a<div className="flex-1 min-h-0">or passingclassNamewithflex-1to ensure it fills the available space.Suggested fix
{selectedBlueprintData?.specDict ? ( + <div className="flex-1 min-h-0"> <GraphDisplay blueprintId={selectedFlow.id} specDict={selectedBlueprintData.specDict} height="100%" showBackground={graphProps?.showBackground} interactive={graphProps?.interactive} centerInView={true} animated={true} validationResults={validationResults} isValidating={isValidating} /> + </div> ) : ( - <div className="flex items-center justify-center h-full text-gray-400"> + <div className="flex-1 flex items-center justify-center text-gray-400"> Loading graph... </div> )}
🤖 Fix all issues with AI agents
In `@ui/client/src/components/agentic-ai/ExecutionTab.tsx`:
- Around line 994-1007: GraphDisplay is being mounted with specDict possibly
undefined (blueprintSpecCache.get(selectedSession.blueprintId)) which causes the
hook inside GraphDisplay to call getBlueprintInfo redundantly; change the render
branch in ExecutionTab so that when selectedSession?.blueprintId exists but
blueprintSpecCache.get(selectedSession.blueprintId) is undefined you render the
same loading/placeholder UI used by WorkflowsPanel (the pattern around
WorkflowsPanel lines 351–367) instead of mounting GraphDisplay; this keeps
GraphDisplay from fetching and waits until fetchResolvedBlueprint (triggered in
handleSessionSelect) populates blueprintSpecCache before passing specDict into
GraphDisplay.
In `@ui/client/src/components/agentic-ai/graphs/GraphDisplayHelpers.ts`:
- Around line 394-396: The fallback for unknown element types in the loop using
layoutNodes -> resolvedElements currently uses CATEGORY_TYPE_TO_PLURAL[el.type]
|| "retrievers", which can incorrectly map unknown types to a real category;
update the logic in GraphDisplayHelpers (the loop referencing layoutNodes,
resolvedElements and CATEGORY_TYPE_TO_PLURAL) to either skip elements when
CATEGORY_TYPE_TO_PLURAL[el.type] is undefined or use a sentinel string such as
"__unknown__" instead of "retrievers" so the unknown type will not match any
real category; ensure downstream code that consumes the category handles the
sentinel (or the absence) safely.
🧹 Nitpick comments (11)
ui/client/src/hooks/use-joint-graph.ts (4)
10-25: Imports split aroundsafeFlushSyncdefinition — consider co-locating.The imports at Lines 10–11 and Lines 25–53 are separated by the
safeFlushSyncfunction definition. While functional, this is an unconventional layout that could confuse readers. Consider movingsafeFlushSyncbelow all imports.
118-161:rebuildOverlayshas an empty dependency array — correct but worth a comment.The
useCallbackcaptures only refs and state setters (all stable identities), so[]is correct. A brief inline comment would save future readers from second-guessing the lint suppression potential.
347-379: Primary color normalization is duplicated.Lines 349 and 377–379 both normalize
primaryHexRef.currentto a hex string, but with slightly different fallback logic. Extract a small helper to unify this.Suggested helper
+const normalizePrimaryHex = (raw: string | undefined): string => { + if (!raw) return "#8b5cf6"; + return raw.startsWith("#") ? raw : `#${raw}`; +}; + // In the async IIFE: -const primaryNow = primaryHexRef.current || "#8b5cf6"; +const primaryNow = normalizePrimaryHex(primaryHexRef.current); ... -const linkColor = primaryHexRef.current?.startsWith("#") - ? primaryHexRef.current - : `#${primaryHexRef.current || "8b5cf6"}`; +const linkColor = normalizePrimaryHex(primaryHexRef.current);
318-491: The async IIFE is ~170 lines — consider extracting the layout + sizing logic.The body from data fetch → node/edge creation → layout → sizing → overlay build is a single contiguous block. Extracting a
buildGraph(graph, layoutNodes, layoutEdges, primaryHex)helper and asizeAndFitPaper(paper, container, bbox, centerInView)helper would improve readability and testability without changing behavior.ui/client/src/components/agentic-ai/graphs/GraphDisplay.tsx (1)
7-13:safeFlushSyncis duplicated inuse-joint-graph.ts(Lines 18–24).Extract into a shared utility (e.g.,
lib/reactUtils.ts) to avoid drift between the two copies.Suggested refactor
// New file: lib/reactUtils.ts +import { flushSync } from "react-dom"; + +/** Runs `fn` synchronously via flushSync; falls back to normal call on error. */ +export function safeFlushSync(fn: () => void): void { + try { + flushSync(fn); + } catch { + fn(); + } +}Then import from both files:
-function safeFlushSync(fn: () => void): void { ... } +import { safeFlushSync } from "@/lib/reactUtils";ui/client/src/components/agentic-ai/WorkflowsPanel.tsx (2)
124-133: Missingeslint-disablefor exhaustive-deps — may produce a lint warning.
fetchAvailableFlowsandfetchActiveFlowsare captured in the effect but not listed in the[user]dependency array. This is semantically correct (re-fetch only on user change), but React's lint rule will flag it. Either add// eslint-disable-next-line react-hooks/exhaustive-depsor wrap the fetch functions inuseCallback.
240-246: Duplicate auto-select logic.
fetchAvailableFlows(Lines 99–101) already auto-selects the first flow when none is selected. This effect duplicates that logic and can cause a redundantonFlowSelectcall on initial mount.ui/client/src/components/agentic-ai/ExecutionTab.tsx (2)
105-106:blueprintSpecCachegrows without eviction.The
Map<string, any>is appended to on every session selection (Line 329–333) but never pruned. In a long-running session with many distinct blueprints, this could accumulate unbounded memory. Consider an LRU bound or clearing the cache on user change.
480-483: Missingeslint-disablefor exhaustive-deps.
fetchChatSessionsis referenced inside the effect but not in the[]dependency array. Consider adding an eslint-disable comment to suppress the inevitable lint warning.ui/client/src/components/agentic-ai/graphs/AgentNodeOverlay.tsx (2)
20-39: Consider extracting the inline prop type into a named interface.The component's props are defined inline in the function signature (Lines 30–39), making it harder to reuse or reference the type externally. A named
AgentNodeOverlayPropsinterface would improve discoverability.
86-110: IIFE for conditional rendering is functional but unusual.The
{hasStatus && (() => { ... })()}pattern is correct but might surprise contributors. A simple{hasStatus && nodeStatus === "PROGRESS" ? <motion.div .../> : <div .../>}would be more idiomatic React.
ui/client/src/components/agentic-ai/graphs/GraphDisplayHelpers.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@ui/client/src/components/agentic-ai/graphs/GraphDisplay.tsx`:
- Around line 320-348: The catch block for fetchResourceById currently only logs
at debug and leaves the user uninformed; replace the console.debug in the .catch
with a user-visible error flow by invoking the app's toast/snackbar utility (or
set an inline error state) to surface the failure, and also ensure you still
call setLoadingResource(false) (it already does in .finally) and keep resource
details closed; update the .catch to log at error level (console.error) and call
something like showToast/enqueueSnackbar or setResourceDetailsError to display a
brief, clear message, referencing fetchResourceById, setLoadingResource,
setResourceDetailsElement, and setResourceDetailsOpen.
🧹 Nitpick comments (6)
ui/client/src/hooks/use-joint-graph.ts (2)
19-55: Imports placed after function definition.
safeFlushSyncis defined at lines 19–26, splitting the import block. Move all imports to the top of the file (before any function definitions) for conventional module structure.
294-318:waitForValidSizetimer is not cleared on cancellation.When the effect cleanup sets
cancelled = true, theResizeObservercallback will disconnect and resolve, but thesetTimeoutat line 313 keeps running. It's benign (callingdisconnect()andresolve()on an already-settled promise are no-ops), but the dangling timer is unnecessary.🧹 Proposed fix
const waitForValidSize = ( el: HTMLElement, minDim = 50, timeoutMs = 3000, ): Promise<boolean> => new Promise((resolve) => { if (el.clientWidth >= minDim && el.clientHeight >= minDim) { resolve(true); return; } + let timer: ReturnType<typeof setTimeout>; const ro = new ResizeObserver(() => { - if (cancelled) { ro.disconnect(); resolve(false); return; } + if (cancelled) { ro.disconnect(); clearTimeout(timer); resolve(false); return; } if (el.clientWidth >= minDim && el.clientHeight >= minDim) { ro.disconnect(); clearTimeout(timer); resolve(true); } }); ro.observe(el); - const timer = setTimeout(() => { + timer = setTimeout(() => { ro.disconnect(); // Last-ditch check — container may be big enough now resolve(el.clientWidth >= minDim && el.clientHeight >= minDim); }, timeoutMs); });ui/client/src/components/agentic-ai/graphs/GraphDisplay.tsx (1)
8-15:safeFlushSyncis duplicated in bothuse-joint-graph.tsandGraphDisplay.tsx.Extract it into a shared utility (e.g.,
utils/safeFlushSync.ts) to keep a single source of truth.ui/client/src/components/agentic-ai/ExecutionTab.tsx (3)
248-261: LocalisSharingDisabledshadows the component-level state variable.The local variable at line 254 has the same name as the state variable declared at line 100 and its setter. This is confusing and could lead to accidental misuse. Rename the local variable (e.g.,
sessionSharingDisabled).
100-106:blueprintSpecCacheas aMapinuseStateis unconventional in React.While the immutable update pattern at line 338–342 (
new Map(prev)) is correct,MapinuseStatecan be error-prone if future contributors mutate it directly. A plainRecord<string, any>would be more idiomatic for React state and avoids the need fornew Map()copies.♻️ Suggested change
- const [blueprintSpecCache, setBlueprintSpecCache] = useState<Map<string, any>>(new Map()); + const [blueprintSpecCache, setBlueprintSpecCache] = useState<Record<string, any>>({});Then update the setter:
- setBlueprintSpecCache(prev => { - const next = new Map(prev); - next.set(session.blueprintId, resolvedBlueprint.spec_dict); - return next; - }); + setBlueprintSpecCache(prev => ({ + ...prev, + [session.blueprintId]: resolvedBlueprint.spec_dict, + }));And the lookup:
- specDict={blueprintSpecCache.get(selectedSession.blueprintId)} + specDict={blueprintSpecCache[selectedSession.blueprintId]}
489-492:fetchChatSessionsreferenced insideuseEffectwithout being in the dependency array.
fetchChatSessionsis not wrapped inuseCallbackand closes over state/props that may change. The empty[]dependency array means it runs once on mount, which is likely intentional, but ESLint'sreact-hooks/exhaustive-depsrule would flag this. Either wrapfetchChatSessionsinuseCallbackwith appropriate deps, or inline the logic in the effect.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
ui/client/src/components/agentic-ai/graphs/AgentNodeOverlay.tsx (1)
86-110: Consider extracting the IIFE inside JSX into a named sub-component or variable.The
{hasStatus && (() => { ... })()}pattern on lines 86–110 (and similarly forstatusPillon lines 48–77) is functional but unusual in React. Extracting these into small named components (e.g.,StatusBorder) or pre-computed variables above the return improves readability and avoids re-creating the closure on every render.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/client/src/components/agentic-ai/graphs/AgentNodeOverlay.tsx` around lines 86 - 110, Extract the inline IIFE used for rendering the status border in AgentNodeOverlay (the `{hasStatus && (() => { ... })()}` block that uses hdr, nodeStatus and STATUS_STYLES) into a small named React component (e.g., StatusBorder) or a pre-computed variable above the component return; move the logic that computes `s` and `borderStyle` out of the JSX so it’s not recreated as a closure each render, accept hdr and nodeStatus as props (or capture them in the variable), and replace the IIFE in the JSX with a simple <StatusBorder .../> or `{borderElem}` reference; do the same refactor for the `statusPill` IIFE so both are readable and avoid recreating closures on every render.
🤖 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/components/agentic-ai/graphs/AgentNodeOverlay.tsx`:
- Around line 133-144: In AgentNodeOverlay adjust the computed maxWidth so it
never goes negative: replace the current expression for maxWidth with a clamped
value using Math.max(0, ...) so when computing maxWidth for hdr (use the same
conditional on hasStatus) you do Math.max(0, hasStatus ? hdr.width - 120 :
hdr.width - 40); update the style block where hdr.label is rendered so maxWidth
is the clamped value.
- Around line 186-189: The onClick handler on the badge currently calls
e.stopPropagation() unconditionally, so even when interactive is false the click
is swallowed; update the handler used in AgentNodeOverlay so that
e.stopPropagation() and onBadgeClick(badge.element.id) only run when interactive
is true (i.e., guard both calls behind if (interactive) { ... }), leaving
non-interactive badges to be transparent to pointer events and allowing the
graph canvas to receive clicks.
In `@ui/client/src/components/agentic-ai/graphs/NodeValidationIndicator.tsx`:
- Around line 67-73: The component NodeValidationIndicator renders an outer div
with a static "cursor-pointer" but only invokes onClick when !isValid, which
misleads users; update the className computation in NodeValidationIndicator (the
JSX that builds `className` using `className`, `isValid`, and current
hover/scale classes) to apply "cursor-pointer" only when the click is actionable
(i.e., when `!isValid && typeof onClick === 'function'`) and use a non-clickable
cursor (e.g., "cursor-default" or omit cursor class) when `isValid` is true so
the pointer only appears for clickable states. Ensure you reference the same
props `isValid` and `onClick` used in the onClick handler when adjusting the
class string.
---
Duplicate comments:
In `@ui/client/src/components/agentic-ai/graphs/NodeValidationIndicator.tsx`:
- Around line 59-65: The tooltipContent currently includes an "0 errors" segment
when errorCount === 0; update the tooltip construction in
NodeValidationIndicator (tooltipContent) to only include the errors segment if
errorCount > 0 and only include the warnings segment if warningCount > 0,
joining them with ", " when both exist; preserve correct pluralization for
"error(s)" and "warning(s)" and ensure the isValid branch still returns
"Validation passed".
---
Nitpick comments:
In `@ui/client/src/components/agentic-ai/graphs/AgentNodeOverlay.tsx`:
- Around line 86-110: Extract the inline IIFE used for rendering the status
border in AgentNodeOverlay (the `{hasStatus && (() => { ... })()}` block that
uses hdr, nodeStatus and STATUS_STYLES) into a small named React component
(e.g., StatusBorder) or a pre-computed variable above the component return; move
the logic that computes `s` and `borderStyle` out of the JSX so it’s not
recreated as a closure each render, accept hdr and nodeStatus as props (or
capture them in the variable), and replace the IIFE in the JSX with a simple
<StatusBorder .../> or `{borderElem}` reference; do the same refactor for the
`statusPill` IIFE so both are readable and avoid recreating closures on every
render.
…to graph-display-new-library
…to graph-display-new-library
MayaCrmi
dismissed stale reviews from sfiresht, Lina-AbuYousef, and Nirsisr
via
a423750
4 participants
Replace ReactFlow with JointJS for workflow graph display
Summary
This PR replaces the ReactFlowGraph component with a new GraphDisplay component, using a different library (JointJS) for displaying the graph than the one we use for building it. It also introduces a centralized theme color system (deriveThemeColors) so all graph-related components derive their colors from a single primary hex instead of using hardcoded emerald/orange/purple values.
Why
The issue of the edge intersections is one we have been trying to fix for some time now. Previously I tried implementing a lot of algorithmic logic to make it work as we want it to using the current library, but after discussions we've decided to try and use a new library for our case.
What changed
Under the graph creation nothing major changed besides coloring - both for the building blocks, including conditions, and the edge color.
The new graph display look:
Core replacement
Other changes
shared/WorkflowStatusBanner.tsx — Split validationFailed into validationFailed (owner-facing) and validationFailedShared (shared-chat-facing) with different messages
package.json — Added @joint/core and @joint/layout-directed-graph dependencies
Cleanup (refactoring pass)
Removed unused showControls prop from GraphDisplayProps and all consumers (JointJS has no built-in controls like ReactFlow did)
Removed unused iconSize variable in badge rendering
Removed openElementDetails from the useEffect dependency array (it was never called inside the effect, causing unnecessary full graph rebuilds on callback reference changes)
Removed dead nodeSuccessors variable from graphFlowLayout.ts (populated but never read)
Extracted GraphDisplayHelpers.ts — all layout constants, overlay interfaces, SVG DOM manipulation (injectSvgDefs, injectLinkAnimations), element block map builder, and category mapping into a dedicated helpers file
Summary by CodeRabbit
New Features
Bug Fixes
Documentation