From 26fe42739b0c388b22c322aed40072ce8d693cc2 Mon Sep 17 00:00:00 2001 From: iliya Date: Sat, 28 Mar 2026 14:41:24 +0200 Subject: [PATCH] refactor(graph): fix SOLID/DRY violations from audit MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit DRY fixes: - GraphOverlay.tsx: 539 LOC → 85 LOC minimal fallback (was dead code) - COLUMN_LABELS: import from COLORS instead of hardcoded hex duplicates - Alpha hex LUT: consolidated to single source in colors.ts - TeamGraphTab: extract typed dispatchOpenTask/dispatchSendMessage helpers (was 8 repeated CustomEvent dispatches) SOLID fixes: - D-1: lucide-react added to peerDependencies in package.json - S-1: GraphOverlay no longer has 8 responsibilities (now just 1 fallback) Architecture: - renderOverlay prop properly used by both Tab and Overlay - Popover rendering lives in features/ layer only (no duplication with package) --- packages/agent-graph/package.json | 3 +- .../agent-graph/src/canvas/render-cache.ts | 7 +- .../agent-graph/src/layout/kanbanLayout.ts | 13 +- packages/agent-graph/src/ui/GraphOverlay.tsx | 518 ++---------------- .../features/agent-graph/ui/TeamGraphTab.tsx | 73 +-- 5 files changed, 69 insertions(+), 545 deletions(-) diff --git a/packages/agent-graph/package.json b/packages/agent-graph/package.json index 2d4eb5c6..0144aa33 100644 --- a/packages/agent-graph/package.json +++ b/packages/agent-graph/package.json @@ -13,7 +13,8 @@ }, "peerDependencies": { "react": "^18.0.0", - "react-dom": "^18.0.0" + "react-dom": "^18.0.0", + "lucide-react": ">=0.300.0" }, "dependencies": { "d3-force": "^3.0.0" diff --git a/packages/agent-graph/src/canvas/render-cache.ts b/packages/agent-graph/src/canvas/render-cache.ts index 9f163e92..7336d497 100644 --- a/packages/agent-graph/src/canvas/render-cache.ts +++ b/packages/agent-graph/src/canvas/render-cache.ts @@ -44,15 +44,14 @@ function hexWithAlpha(color: string, alpha: number): string { const key = `${color}|${a}`; let result = _hexAlphaCache.get(key); if (result) return result; - result = ensureHex(color) + ALPHA_LUT[a]; + result = ensureHex(color) + alphaHex(a / 255); _hexAlphaCache.set(key, result); if (_hexAlphaCache.size > 500) _hexAlphaCache.clear(); // prevent unbounded growth return result; } -// Import-time LUT for alpha hex -const ALPHA_LUT: string[] = []; -for (let i = 0; i < 256; i++) ALPHA_LUT.push(i.toString(16).padStart(2, '0')); +// Reuse alpha hex LUT from colors.ts (DRY — single source) +import { alphaHex } from '../constants/colors'; // ─── Glow Sprite Cache ────────────────────────────────────────────────────── diff --git a/packages/agent-graph/src/layout/kanbanLayout.ts b/packages/agent-graph/src/layout/kanbanLayout.ts index 5be02b2c..d28885d8 100644 --- a/packages/agent-graph/src/layout/kanbanLayout.ts +++ b/packages/agent-graph/src/layout/kanbanLayout.ts @@ -9,6 +9,7 @@ import type { GraphNode } from '../ports/types'; import { KANBAN_ZONE } from '../constants/canvas-constants'; +import { COLORS } from '../constants/colors'; /** Column header info for rendering */ export interface KanbanColumnHeader { @@ -26,13 +27,13 @@ export interface KanbanZoneInfo { headers: KanbanColumnHeader[]; } -// Column display config +// Column display config — colors from single source of truth (COLORS) const COLUMN_LABELS: Record = { - todo: { label: 'Todo', color: '#6b7280' }, - wip: { label: 'In Progress', color: '#3b82f6' }, - done: { label: 'Done', color: '#22c55e' }, - review: { label: 'Review', color: '#f59e0b' }, - approved: { label: 'Approved', color: '#22c55e' }, + todo: { label: 'Todo', color: COLORS.taskPending }, + wip: { label: 'In Progress', color: COLORS.taskInProgress }, + done: { label: 'Done', color: COLORS.taskCompleted }, + review: { label: 'Review', color: COLORS.reviewPending }, + approved: { label: 'Approved', color: COLORS.reviewApproved }, }; export class KanbanLayoutEngine { diff --git a/packages/agent-graph/src/ui/GraphOverlay.tsx b/packages/agent-graph/src/ui/GraphOverlay.tsx index 7922c619..301e6998 100644 --- a/packages/agent-graph/src/ui/GraphOverlay.tsx +++ b/packages/agent-graph/src/ui/GraphOverlay.tsx @@ -1,15 +1,11 @@ /** - * GraphOverlay — HTML popovers positioned over Canvas nodes. - * Uses camera worldToScreen transform for positioning. - * - * Styled to match the host app's MemberHoverCard / MemberCard look: - * avatar + status dot, name, role, status badges, action buttons. + * GraphOverlay — minimal built-in popover fallback. + * Used ONLY when host app doesn't provide renderOverlay prop. + * For full-featured popovers, use renderOverlay with project UI components. */ -import { useCallback, useEffect, useRef } from 'react'; import type { GraphNode } from '../ports/types'; import type { GraphEventPort } from '../ports/GraphEventPort'; -import { COLORS, getStateColor, getTaskStatusColor } from '../constants/colors'; export interface GraphOverlayProps { selectedNode: GraphNode | null; @@ -37,502 +33,56 @@ export function GraphOverlay({ transform: 'translateY(-50%)', }} > - - - ); -} - -// ─── SVG Icons (inline — package cannot import lucide-react) ──────────────── - -function IconMessage({ size = 13 }: { size?: number }): React.JSX.Element { - return ( - - - - ); -} - -function IconExternalLink({ size = 12 }: { size?: number }): React.JSX.Element { - return ( - - - - - - ); -} - -function IconGlobe({ size = 12 }: { size?: number }): React.JSX.Element { - return ( - - - - - - ); -} - -function IconClipboard({ size = 12 }: { size?: number }): React.JSX.Element { - return ( - - - - - ); -} - -// ─── State helpers ────────────────────────────────────────────────────────── - -function getPresenceLabel(state: GraphNode['state']): string { - switch (state) { - case 'active': return 'active'; - case 'thinking': return 'thinking'; - case 'tool_calling': return 'tool calling'; - case 'idle': return 'idle'; - case 'waiting': return 'waiting'; - case 'complete': return 'done'; - case 'error': return 'error'; - case 'terminated': return 'offline'; - } -} - -function getStatusDotClass(state: GraphNode['state']): string { - switch (state) { - case 'active': - case 'thinking': - case 'tool_calling': - return 'animate-pulse'; - default: - return ''; - } -} - -/** Capitalise first letter, replace underscores with spaces */ -function formatLabel(s: string): string { - return s.replace(/_/g, ' ').replace(/^\w/, (c) => c.toUpperCase()); -} - -/** Truncate display name: "team-lead" → "Team Lead", "alice" → "Alice" */ -function displayName(name: string): string { - return name - .split(/[-_]/) - .map((w) => w.charAt(0).toUpperCase() + w.slice(1)) - .join(' '); -} - -// ─── Node Popover ─────────────────────────────────────────────────────────── - -function NodePopover({ - node, - events, - onClose, -}: { - node: GraphNode; - events?: GraphEventPort; - onClose: () => void; -}): React.JSX.Element { - const popoverRef = useRef(null); - - // Close on outside click - useEffect(() => { - const handler = (e: MouseEvent) => { - if (popoverRef.current && !popoverRef.current.contains(e.target as Node)) { - onClose(); - } - }; - // Delay to avoid closing immediately from the click that opened it - const timer = setTimeout(() => document.addEventListener('mousedown', handler), 50); - return () => { - clearTimeout(timer); - document.removeEventListener('mousedown', handler); - }; - }, [onClose]); - - const handleAction = useCallback( - (action: string) => { - const ref = node.domainRef; - switch (action) { - case 'sendMessage': - if (ref.kind === 'member' || ref.kind === 'lead') { - events?.onSendMessage?.(ref.kind === 'member' ? ref.memberName : 'team-lead', ref.teamName); - } - break; - case 'openDetail': - if (ref.kind === 'task') events?.onOpenTaskDetail?.(ref.taskId, ref.teamName); - else if (ref.kind === 'member') events?.onOpenMemberProfile?.(ref.memberName, ref.teamName); - else if (ref.kind === 'lead') events?.onOpenMemberProfile?.('team-lead', ref.teamName); - break; - case 'openUrl': - if (node.processUrl) window.open(node.processUrl, '_blank'); - break; - } - onClose(); - }, - [node, events, onClose], - ); - - const isMemberLike = node.kind === 'member' || node.kind === 'lead'; - const color = node.kind === 'task' - ? getTaskStatusColor(node.taskStatus) - : node.color ?? getStateColor(node.state); - const stateColor = getStateColor(node.state); - - if (isMemberLike) { - return ; - } - if (node.kind === 'task') { - return ; - } - return ; -} - -// ─── Member / Lead Popover ────────────────────────────────────────────────── - -import { forwardRef } from 'react'; - -const MemberPopover = forwardRef< - HTMLDivElement, - { node: GraphNode; color: string; stateColor: string; onAction: (a: string) => void } ->(function MemberPopover({ node, color, stateColor, onAction }, ref) { - const presenceText = getPresenceLabel(node.state); - const dotAnim = getStatusDotClass(node.state); - - return ( -
- {/* Colored top accent */} -
- -
- {/* Header: avatar + name + status */} -
- {node.avatarUrl ? ( -
- {node.label} - -
- ) : ( -
-
- {node.label.charAt(0).toUpperCase()} -
- -
- )} - -
-
- - {displayName(node.label)} - -
- {node.role && ( -
- {node.role} -
- )} -
+
+
+ {selectedNode.label}
- - {/* Status badge */} -
- - {node.kind === 'lead' && ( - - )} - {node.spawnStatus && node.spawnStatus !== 'online' && ( - - )} -
- - {/* Context usage bar (lead only) */} - {node.contextUsage != null && node.contextUsage > 0 && ( -
-
- Context - {Math.round(node.contextUsage * 100)}% -
-
-
0.8 ? COLORS.error : color, - }} - /> -
+ {selectedNode.sublabel && ( +
+ {selectedNode.sublabel}
)} - - {/* Sublabel (current activity) */} - {node.sublabel && ( -
- {node.sublabel} + {selectedNode.role && ( +
+ {selectedNode.role}
)} - - {/* Action buttons */} -
- } label="Message" onClick={() => onAction('sendMessage')} color={color} /> - } label="Profile" onClick={() => onAction('openDetail')} color={color} /> -
-
-
- ); -}); - -// ─── Task Popover ─────────────────────────────────────────────────────────── - -const TaskPopover = forwardRef< - HTMLDivElement, - { node: GraphNode; color: string; stateColor: string; onAction: (a: string) => void } ->(function TaskPopover({ node, color, stateColor, onAction }, ref) { - const taskStatusLabel = node.taskStatus ? formatLabel(node.taskStatus) : 'pending'; - - return ( -
- {/* Colored top accent */} -
- -
- {/* Header: display ID + label */} -
- {node.displayId && ( - - {node.displayId} - - )} - - {node.label} - -
- - {/* Subject / description */} - {node.sublabel && ( -
- {node.sublabel} -
- )} - - {/* Status badges */} -
- - {node.state !== 'idle' && ( - - )} - {node.reviewState && node.reviewState !== 'none' && ( - + {(selectedNode.kind === 'member' || selectedNode.kind === 'lead') && ( + { + const ref = selectedNode.domainRef; + if (ref.kind === 'member') events?.onSendMessage?.(ref.memberName, ref.teamName); + onDeselect(); + }} /> )} - {node.needsClarification && ( - - )} -
- - {/* Action */} -
- } label="Open task" onClick={() => onAction('openDetail')} color={color} /> +
); -}); - -// ─── Process Popover ──────────────────────────────────────────────────────── - -const ProcessPopover = forwardRef< - HTMLDivElement, - { node: GraphNode; color: string; onAction: (a: string) => void } ->(function ProcessPopover({ node, color, onAction }, ref) { - return ( -
-
- -
-
-
- - {node.label} - -
- - {node.sublabel && ( -
- {node.sublabel} -
- )} - -
- -
- - {node.processUrl && ( -
- } label="Open URL" onClick={() => onAction('openUrl')} color={color} /> -
- )} -
-
- ); -}); - -// ─── UI Primitives ────────────────────────────────────────────────────────── - -function StatusBadge({ label, color }: { label: string; color: string }): React.JSX.Element { - return ( - - {label} - - ); } -function ActionButton({ - icon, - label, - onClick, - color, -}: { - icon: React.ReactNode; - label: string; - onClick: () => void; - color: string; -}): React.JSX.Element { +function FallbackButton({ label, onClick }: { label: string; onClick: () => void }): React.JSX.Element { return ( ); diff --git a/src/renderer/features/agent-graph/ui/TeamGraphTab.tsx b/src/renderer/features/agent-graph/ui/TeamGraphTab.tsx index ba1a0842..8f634922 100644 --- a/src/renderer/features/agent-graph/ui/TeamGraphTab.tsx +++ b/src/renderer/features/agent-graph/ui/TeamGraphTab.tsx @@ -24,46 +24,31 @@ export const TeamGraphTab = ({ teamName }: TeamGraphTabProps): React.JSX.Element const graphData = useTeamGraphAdapter(teamName); const [fullscreen, setFullscreen] = useState(false); + // Typed event dispatchers (DRY — used in both events + renderOverlay) + const dispatchOpenTask = useCallback( + (taskId: string) => + window.dispatchEvent(new CustomEvent('graph:open-task', { detail: { teamName, taskId } })), + [teamName] + ); + const dispatchSendMessage = useCallback( + (memberName: string) => + window.dispatchEvent( + new CustomEvent('graph:send-message', { detail: { teamName, memberName } }) + ), + [teamName] + ); + const events: GraphEventPort = { onNodeDoubleClick: useCallback( (ref: GraphDomainRef) => { - // Dispatch to TeamDetailView's dialog system via CustomEvent - if (ref.kind === 'task') { - window.dispatchEvent( - new CustomEvent('graph:open-task', { detail: { teamName, taskId: ref.taskId } }) - ); - } else if (ref.kind === 'member') { - window.dispatchEvent( - new CustomEvent('graph:send-message', { - detail: { teamName, memberName: ref.memberName }, - }) - ); - } + if (ref.kind === 'task') dispatchOpenTask(ref.taskId); + else if (ref.kind === 'member') dispatchSendMessage(ref.memberName); }, - [teamName] - ), - onSendMessage: useCallback( - (memberName: string) => { - window.dispatchEvent( - new CustomEvent('graph:send-message', { detail: { teamName, memberName } }) - ); - }, - [teamName] - ), - onOpenTaskDetail: useCallback( - (taskId: string) => { - window.dispatchEvent(new CustomEvent('graph:open-task', { detail: { teamName, taskId } })); - }, - [teamName] - ), - onOpenMemberProfile: useCallback( - (memberName: string) => { - window.dispatchEvent( - new CustomEvent('graph:send-message', { detail: { teamName, memberName } }) - ); - }, - [teamName] + [dispatchOpenTask, dispatchSendMessage] ), + onSendMessage: dispatchSendMessage, + onOpenTaskDetail: dispatchOpenTask, + onOpenMemberProfile: dispatchSendMessage, }; return ( @@ -77,21 +62,9 @@ export const TeamGraphTab = ({ teamName }: TeamGraphTabProps): React.JSX.Element - window.dispatchEvent( - new CustomEvent('graph:send-message', { detail: { teamName, memberName: name } }) - ) - } - onOpenTaskDetail={(id) => - window.dispatchEvent( - new CustomEvent('graph:open-task', { detail: { teamName, taskId: id } }) - ) - } - onOpenMemberProfile={(name) => - window.dispatchEvent( - new CustomEvent('graph:send-message', { detail: { teamName, memberName: name } }) - ) - } + onSendMessage={dispatchSendMessage} + onOpenTaskDetail={dispatchOpenTask} + onOpenMemberProfile={dispatchSendMessage} /> )} />