Conversation
📝 WalkthroughWalkthroughAdds explicit graph visibility prop, defensive try/catch logging around SVG matrix operations, and logic to reapply node visuals when a previously hidden graph becomes visible; also includes several UI styling/theme tweaks and button/tooltip logic updates. Changes
Sequence Diagram(s)sequenceDiagram
participant Exec as ExecutionTab (UI)
participant Graph as GraphDisplay
participant NodeStore as nodeStatusMapRef
participant Joint as JointJS Graph
Exec->>Graph: render with isGraphVisible (true/false)
note right of Graph: GraphDisplay monitors visibility
Graph->>Graph: detect transition hidden -> visible
Graph->>NodeStore: iterate stored node visuals
NodeStore-->>Graph: provide visuals per node
Graph->>Joint: applyNodeVisual(node, visual)
alt SVG matrix invertible
Joint-->>Graph: attributes updated
else non-invertible
Joint-->>Graph: throws -> caught, warn logged
end
Graph->>Graph: schedule/confirm reapply complete
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ 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 |
🔍 SummaryThis Pull Request addresses critical runtime errors in the JointJS graph visualization ( The changes align with the Agentic AI System architecture, specifically the read-only / view-mode workflow display strategy using JointJS. 🧩 File-by-file feedback[DOMAIN: UI] ui/client/src/components/agentic-ai/graphs/GraphDisplay.tsx
[DOMAIN: UI] ui/client/src/hooks/use-joint-graph.ts
[DOMAIN: UI] ui/client/src/components/agentic-ai/workspace/CategorySidebar.tsx
🛠 Suggested Improvements1. Decouple Timing Logic (GraphDisplay.tsx) // Define at top of file or imported from constants
const GRAPH_RESIZE_DEBOUNCE_MS = 10;
const VISUAL_UPDATE_BUFFER_MS = 50; // Buffer to ensure resize completes
const RESTORE_VISUALS_DELAY = GRAPH_RESIZE_DEBOUNCE_MS + VISUAL_UPDATE_BUFFER_MS;
// ... inside useEffect
pendingTimer = setTimeout(() => {
// logic
}, RESTORE_VISUALS_DELAY);2. Use Semantic Tokens for Styling (CategorySidebar.tsx) // Recommended change using semantic tokens
className={cn(
"w-full justify-start px-4 py-2 rounded-none text-xs",
selectedElementType?.type === elementType.type &&
selectedElementType?.category === elementType.category
? "bg-primary/20 border-r-2 border-primary text-primary" // text-primary ensures contrast on primary/20
: "text-muted-foreground hover:text-foreground"
)}Note: ✅ What's Good
✍️ Suggested Commit Message |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
ui/client/src/components/agentic-ai/graphs/GraphDisplay.tsx (2)
343-343:graphRefandcontainerRefare stable refs — remove them from the dep array.Both
graphRefandcontainerRefareuseRefobjects whose identity never changes across renders. Listing them as deps implies the effect re-runs when they "change" — which never happens — misleading future readers.♻️ Suggested fix
- }, [graphRef, containerRef, applyNodeVisual]); + }, [applyNodeVisual]);🤖 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/GraphDisplay.tsx` at line 343, The effect's dependency array includes stable refs graphRef and containerRef which never change; remove graphRef and containerRef from the deps and only include real changing dependencies (e.g., applyNodeVisual) so the effect runs correctly; update the useEffect dependency list in the component containing the effect that references graphRef, containerRef, and applyNodeVisual to remove the two refs, and if ESLint warns, add a focused comment explaining that graphRef/containerRef are stable refs and intentionally omitted.
325-333: The 60ms delay is implicitly coupled to the 10ms debounce inuse-joint-graph.ts.The value
60exists solely to outlast the 10ms debounce in the sibling file's ResizeObserver (line 541 ofuse-joint-graph.ts). There is no shared constant tying them together, so changing the debounce inuse-joint-graph.tssilently breaks the timing assumption here.Consider extracting a shared constant (e.g.
RESIZE_DEBOUNCE_MS = 10) and deriving the delay from it, or at minimum adding a cross-file reference in the comment.♻️ Suggested approach (shared constant)
In
use-joint-graph.ts(or a shared constants file):+export const RESIZE_DEBOUNCE_MS = 10;In
use-joint-graph.tsResizeObserver:- }, 10); + }, RESIZE_DEBOUNCE_MS);In
GraphDisplay.tsx:+import { RESIZE_DEBOUNCE_MS } from "@/hooks/use-joint-graph"; ... - }, 60); + // Wait for use-joint-graph ResizeObserver (RESIZE_DEBOUNCE_MS + render budget) + }, RESIZE_DEBOUNCE_MS + 50);🤖 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/GraphDisplay.tsx` around lines 325 - 333, The 60ms timeout in GraphDisplay (pendingTimer setTimeout) is implicitly tied to the 10ms debounce in use-joint-graph.ts's ResizeObserver and should be decoupled; introduce a shared constant (e.g. RESIZE_DEBOUNCE_MS) exported from use-joint-graph.ts or a shared constants module and use a derived delay (e.g. RESIZE_DEBOUNCE_MS * N or RESIZE_DEBOUNCE_MS + buffer) when scheduling pendingTimer in GraphDisplay instead of the hard-coded 60, and update the comment to reference the shared constant so applyNodeVisual, graphRef.current and nodeStatusMapRef.current behavior remains correct if the debounce changes.ui/client/src/hooks/use-joint-graph.ts (1)
509-521: Try-catch wraps the entire loop — first throw skips remaining links.If any
link.attr()throws, the remaining links in the iteration won't be updated (partial or zero update depending on which link throws first). Because all links share the same paper SVG context they'll typically all fail or all succeed, so the practical impact is low — but moving the guard inside the loop is more defensive and consistent with theapplyNodeVisualpattern inGraphDisplay.tsx.♻️ Suggested refactor
- try { - for (const link of graph.getLinks()) { - if (conditionalLinkIdsRef.current.has(link.id as string)) continue; - link.attr("line/stroke", linkColor); - link.attr("line/sourceMarker/fill", linkColor); - link.attr("line/targetMarker/fill", linkColor); - } - } catch { - // SVGMatrix non-invertible when the container is collapsed to zero - // width (e.g. carousel chat mode). The ResizeObserver will re-fit - // the paper once the container becomes visible; link colors are - // cosmetic and will be correct on next full rebuild. - } + for (const link of graph.getLinks()) { + if (conditionalLinkIdsRef.current.has(link.id as string)) continue; + try { + link.attr("line/stroke", linkColor); + link.attr("line/sourceMarker/fill", linkColor); + link.attr("line/targetMarker/fill", linkColor); + } catch { + // SVGMatrix non-invertible — container is collapsed (e.g. carousel + // chat mode). Colors will be correct on next full rebuild once the + // ResizeObserver re-fits the paper. + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/client/src/hooks/use-joint-graph.ts` around lines 509 - 521, The current try-catch around the whole loop means a single link.attr() exception aborts updating remaining links; change it to catch per-link so other links still get updated: iterate over graph.getLinks(), skip when conditionalLinkIdsRef.current.has(link.id as string) as before, then wrap the link.attr(...) calls for each link in a try-catch (mirroring applyNodeVisual in GraphDisplay.tsx) so SVGMatrix errors only skip that one link and do not prevent processing of subsequent links.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@ui/client/src/components/agentic-ai/graphs/GraphDisplay.tsx`:
- Line 343: The effect's dependency array includes stable refs graphRef and
containerRef which never change; remove graphRef and containerRef from the deps
and only include real changing dependencies (e.g., applyNodeVisual) so the
effect runs correctly; update the useEffect dependency list in the component
containing the effect that references graphRef, containerRef, and
applyNodeVisual to remove the two refs, and if ESLint warns, add a focused
comment explaining that graphRef/containerRef are stable refs and intentionally
omitted.
- Around line 325-333: The 60ms timeout in GraphDisplay (pendingTimer
setTimeout) is implicitly tied to the 10ms debounce in use-joint-graph.ts's
ResizeObserver and should be decoupled; introduce a shared constant (e.g.
RESIZE_DEBOUNCE_MS) exported from use-joint-graph.ts or a shared constants
module and use a derived delay (e.g. RESIZE_DEBOUNCE_MS * N or
RESIZE_DEBOUNCE_MS + buffer) when scheduling pendingTimer in GraphDisplay
instead of the hard-coded 60, and update the comment to reference the shared
constant so applyNodeVisual, graphRef.current and nodeStatusMapRef.current
behavior remains correct if the debounce changes.
In `@ui/client/src/hooks/use-joint-graph.ts`:
- Around line 509-521: The current try-catch around the whole loop means a
single link.attr() exception aborts updating remaining links; change it to catch
per-link so other links still get updated: iterate over graph.getLinks(), skip
when conditionalLinkIdsRef.current.has(link.id as string) as before, then wrap
the link.attr(...) calls for each link in a try-catch (mirroring applyNodeVisual
in GraphDisplay.tsx) so SVGMatrix errors only skip that one link and do not
prevent processing of subsequent links.
There was a problem hiding this comment.
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/pages/AgenticWorkflows.tsx (1)
145-155:⚠️ Potential issue | 🔴 CriticalTooltip on disabled button will not display to users.
The new
!selectedFlowbranch returns<p>Select a workflow first</p>, but theButtoncomponent appliesdisabled:pointer-events-nonewhen disabled. Since the button is disabled exactly when!selectedFlowis true, pointer events will not fire and the tooltip will be invisible.SimpleTooltip uses
asChildon the TooltipTrigger, which means trigger behavior is applied directly to the Button element. When that Button haspointer-events: none, the Radix UI tooltip cannot detect hover events.This issue applies equally to the pre-existing tooltip branches (
!isFlowValidandisValidatingFlow), but this PR introduces a new user-facing message that will silently fail to display.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/client/src/pages/AgenticWorkflows.tsx` around lines 145 - 155, The tooltip never appears when the Button is disabled because the Button gets disabled:pointer-events-none and SimpleTooltip uses asChild (so the trigger is the Button); update the JSX around SimpleTooltip and the Button in AgenticWorkflows.tsx to wrap the Button with a non-disabled wrapper element (e.g., a <span className="inline-block">) and make the tooltip trigger target that wrapper instead of the Button (or remove asChild and render the TooltipTrigger around the wrapper), so pointer events are received even when the Button is disabled; keep the same conditional content logic for the tooltip.
🧹 Nitpick comments (1)
ui/client/src/pages/AgenticWorkflows.tsx (1)
60-65:handleLoadFlowlacks a guard for!selectedFlow.The button's
disabledprop prevents normal invocation, buthandleLoadFlowstill silently creates a random session (randomgraph-${Date.now()}ID, random name) whenselectedFlowisnull. Adding an early return keeps the function self-consistent with the new UI constraint.🛡️ Proposed defensive guard
const handleLoadFlow = async () => { if (isLoadingFlow) return; // Prevent multiple calls + if (!selectedFlow) return;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/client/src/pages/AgenticWorkflows.tsx` around lines 60 - 65, handleLoadFlow currently creates a random session when selectedFlow is null; add a defensive early-return at the start of handleLoadFlow to exit immediately if !selectedFlow (and ensure isLoadingFlow state is cleared by not setting it to true or by calling setIsLoadingFlow(false) before returning) so the function respects the UI disabled state; reference handleLoadFlow, selectedFlow, isLoadingFlow and setIsLoadingFlow when making the change.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
ui/client/src/components/agentic-ai/workspace/AgentCardVisualization.tsxui/client/src/pages/AgenticWorkflows.tsx
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@ui/client/src/pages/AgenticWorkflows.tsx`:
- Around line 145-155: The tooltip never appears when the Button is disabled
because the Button gets disabled:pointer-events-none and SimpleTooltip uses
asChild (so the trigger is the Button); update the JSX around SimpleTooltip and
the Button in AgenticWorkflows.tsx to wrap the Button with a non-disabled
wrapper element (e.g., a <span className="inline-block">) and make the tooltip
trigger target that wrapper instead of the Button (or remove asChild and render
the TooltipTrigger around the wrapper), so pointer events are received even when
the Button is disabled; keep the same conditional content logic for the tooltip.
---
Nitpick comments:
In `@ui/client/src/pages/AgenticWorkflows.tsx`:
- Around line 60-65: handleLoadFlow currently creates a random session when
selectedFlow is null; add a defensive early-return at the start of
handleLoadFlow to exit immediately if !selectedFlow (and ensure isLoadingFlow
state is cleared by not setting it to true or by calling setIsLoadingFlow(false)
before returning) so the function respects the UI disabled state; reference
handleLoadFlow, selectedFlow, isLoadingFlow and setIsLoadingFlow when making the
change.
…to GENIE-1307/bugfix/svg-black-screen-bug
There was a problem hiding this comment.
♻️ Duplicate comments (1)
ui/client/src/components/agentic-ai/ExecutionTab.tsx (1)
1016-1016:⚠️ Potential issue | 🟠 Major
isGraphVisibleshould reflect effective graph width, not only carousel mode.With Line 202 allowing
maxChatInterface = 100, users can collapse graph width to0while still innormalmode. In that state this prop remainstrue, so the hidden→visible replay path won’t run when width comes back, leaving stale node visuals possible.Proposed fix
- isGraphVisible={carouselMode !== 'chat'} + isGraphVisible={ + !isChatOnlyMode && + carouselMode !== 'chat' && + blueprintGraphWidth > 0 + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ui/client/src/components/agentic-ai/ExecutionTab.tsx` at line 1016, The isGraphVisible prop should be driven by the effective graph width, not just carouselMode; update the prop assignment in ExecutionTab.tsx to compute the graph width (e.g., graphWidth = 100 - maxChatInterface or from the existing chat width state) and set isGraphVisible to (carouselMode !== 'chat' && graphWidth > 0) so the replay/visibility path runs correctly when width is restored; change the single-line prop where isGraphVisible is set to use this combined condition (referencing carouselMode and maxChatInterface/chat width variable).
🧹 Nitpick comments (1)
ui/client/src/components/agentic-ai/graphs/GraphDisplay.tsx (1)
208-214: Avoid repeated exception/log churn while the panel is collapsed.This catch can run on every node update tick when the container is zero-width. Consider short-circuiting visual writes while hidden (or logging once) to reduce hot-path overhead.
Suggested refinement
+ const collapsedVisualWarnedRef = useRef(false); + const applyNodeVisual = useCallback( (el: dia.Element, status: NodeStatus | undefined) => { const s = STATUS_STYLES[status ?? "IDLE"]; try { el.attr("body/stroke", s.stroke); el.attr("body/strokeWidth", s.strokeWidth); el.attr("body/filter", s.filter); + collapsedVisualWarnedRef.current = false; } catch (e) { - console.warn("[GraphDisplay] applyNodeVisual: SVGMatrix non-invertible (container likely collapsed to zero width)", e); + if (!collapsedVisualWarnedRef.current) { + console.warn("[GraphDisplay] applyNodeVisual: SVGMatrix non-invertible (container likely collapsed to zero width)", e); + collapsedVisualWarnedRef.current = true; + } } }, [], );🤖 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/GraphDisplay.tsx` around lines 208 - 214, The catch in applyNodeVisual is hitting repeatedly when the SVG container is collapsed; short-circuit the visual updates instead of repeatedly calling el.attr or spamming console.warn: detect container visibility/size (e.g., check containerRef or graph root boundingClientRect width/height or a CSS visibility flag) before performing the three el.attr calls, and only attempt the visual writes when size > 0; alternatively, if a one-time log is preferred, add a module-level flag (e.g., hasLoggedNonInvertible) and only call console.warn once from the catch in applyNodeVisual to avoid hot-path churn.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@ui/client/src/components/agentic-ai/ExecutionTab.tsx`:
- Line 1016: The isGraphVisible prop should be driven by the effective graph
width, not just carouselMode; update the prop assignment in ExecutionTab.tsx to
compute the graph width (e.g., graphWidth = 100 - maxChatInterface or from the
existing chat width state) and set isGraphVisible to (carouselMode !== 'chat' &&
graphWidth > 0) so the replay/visibility path runs correctly when width is
restored; change the single-line prop where isGraphVisible is set to use this
combined condition (referencing carouselMode and maxChatInterface/chat width
variable).
---
Nitpick comments:
In `@ui/client/src/components/agentic-ai/graphs/GraphDisplay.tsx`:
- Around line 208-214: The catch in applyNodeVisual is hitting repeatedly when
the SVG container is collapsed; short-circuit the visual updates instead of
repeatedly calling el.attr or spamming console.warn: detect container
visibility/size (e.g., check containerRef or graph root boundingClientRect
width/height or a CSS visibility flag) before performing the three el.attr
calls, and only attempt the visual writes when size > 0; alternatively, if a
one-time log is preferred, add a module-level flag (e.g.,
hasLoggedNonInvertible) and only call console.warn once from the catch in
applyNodeVisual to avoid hot-path churn.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
ui/client/src/components/agentic-ai/ExecutionTab.tsxui/client/src/components/agentic-ai/graphs/GraphDisplay.tsxui/client/src/hooks/use-joint-graph.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- ui/client/src/hooks/use-joint-graph.ts
5 participants
Fix for the black screen bug!
After being able to replicate it, I found out that what happened is while we were in chat only display mode, the graph panel is set to width: 0 but GraphDisplay stays mounted (to preserve streaming node state, which we have actually opened a ticket to improve this logic and try and keep this without actually keeping the graph mounted behind the scenes). So when the run finishes, the state of isLiveRequest turns false, triggers a useEffect that calls applyNodeVisual on every element. Each el.attr() call makes JointJS recalculate, which requires computing SVG boxes using the function inverse... But in this particular case (the live streaming finishes while on the chat only mode), the SVG currently lives inside a container with width zero, so the transformation matrix is all zeros and cannot be inverted (some real-life CS degree issues lol). This error crashed the entire React component tree, which gave us the black screen.
The fix for this was in two parts. First, The el.attr() is now wrapped inside applyNodeVisual with a try-catch blick. Now, if this happens, the error is caught silently so the app doesn’t break. The node status state (nodeStatusMapRef) still updates correctly either way. But to fix it from the core of it, I added a ResizeObserver on the graph container, which watches for when the container goes from collapsed option back to big size like for example, when switching from chat-only back to graph mode. When that happens, it re-applies all the current node statuses from nodeStatusMapRef. That way, even if the visual updates failed while the panel was hidden, the graph refreshes well and shows the right DONE / PROGRESS / IDLE styles once it’s visible again.
You'll also see here tiny UI changes I also added על הדרך that were requested:
Summary by CodeRabbit
Bug Fixes
New Features
Style