From e29c5d643c6e9932e8621afc7cbc106258a7f95f Mon Sep 17 00:00:00 2001 From: dtoro Date: Thu, 12 Mar 2026 17:07:42 +0100 Subject: [PATCH] refactoring --- frontend/docs/IMPROVEMENTS.md | 47 ++++++++++++++++ frontend/src/components/graph/NodeMenubar.tsx | 5 ++ .../components/nodes/config/descriptor.tsx | 2 +- .../components/nodes/config/renderingLogic.ts | 42 ++------------- frontend/src/components/nodes/render/index.ts | 4 +- ...enuRegistry.tsx => outputMenuHandlers.tsx} | 0 .../nodes/render/useRenderingNodeState.ts | 49 ++++------------- frontend/src/lib/graph/nodeTypes.ts | 4 +- frontend/src/lib/graph/rendering.ts | 14 +++-- .../src/lib/graph/sourceRenderingLogic.ts | 2 +- frontend/src/lib/graph/templateRefs.ts | 54 +++++++++++++++++++ 11 files changed, 136 insertions(+), 87 deletions(-) create mode 100644 frontend/docs/IMPROVEMENTS.md rename frontend/src/components/nodes/render/{outputMenuRegistry.tsx => outputMenuHandlers.tsx} (100%) create mode 100644 frontend/src/lib/graph/templateRefs.ts diff --git a/frontend/docs/IMPROVEMENTS.md b/frontend/docs/IMPROVEMENTS.md new file mode 100644 index 0000000..42f5568 --- /dev/null +++ b/frontend/docs/IMPROVEMENTS.md @@ -0,0 +1,47 @@ +# Codebase simplification and design patterns + +This doc summarizes recent improvements and suggested next steps for readability, extension, and consistency. + +## Done + +### 1. Single place for template/reachability (DRY) + +- **Added** `lib/graph/templateRefs.ts`: `isReachable`, `resolveExtendsRef`, `getTemplateRefs`. +- **Refactored** `config/renderingLogic.ts` and `useRenderingNodeState.ts` to use these helpers instead of duplicating the same logic. +- **Pattern:** Extract shared pure helpers into a small lib module; keep call sites thin and consistent. + +### 2. Naming and comments + +- **Renamed** `outputMenuRegistry.tsx` → `outputMenuHandlers.tsx` (no registry, only helpers). +- **Updated** `rendering.ts` and `sourceRenderingLogic.ts`: output menu is described as coming from the node descriptor (`getOutputMenuContent`), not a separate registry. +- **Documented** `NodeMenubar`: extra content can come from props or from the descriptor (`getNodeMenuExtraContent`). +- **Documented** `nodeTypes.ts`: clarifies React Flow types vs node type id (nodeRegistry). + +### 3. Central pipeline entry + +- **rendering.ts** documents the 3-step pipeline, how to add a source/output type, and points to `templateRefs.ts` for shared helpers. + +## Design patterns in use + +| Pattern | Where | +|----------------|--------------------------------------------| +| **Registry** | nodeRegistry, sourceRenderingLogic | +| **Builder** | nodeTypeBuilder (descriptor per node type) | +| **Pipeline** | Resolve → Render → Display (rendering.ts) | +| **Strategy** | Source logic per node type; output menu per descriptor | +| **Shared helpers** | templateRefs, outputMenuHandlers, renderingUtils | + +## Suggested next steps + +1. **CanvasPage** (~950 lines): Split into smaller units, e.g.: + - `useCanvasGraph()` or similar for graph state and connection rules. + - A dedicated component for the context menu (add node, paste, etc.). + - Keeps CanvasPage as composition + layout. + +2. **useRenderingNodeState**: Consider extracting: + - Signature building (config/edges/variables/functions/data) into a pure function or small module, e.g. `buildSourceSignatures(nodes, edges, id, incomingIds, getConfigContent)`. + - Makes the hook easier to read and the logic testable in isolation. + +3. **Config types**: If you add more output types (e.g. Mermaid), consider a small registry API (`registerConfigType`, `getConfigType`) instead of a single large `CONFIG_TYPES` array, so extensions can register without editing the core list. + +4. **Consistent node shape in lib**: `templateRefs` uses `EdgeLike` / `NodeLike`; other graph code uses inline `{ source, target }` or `nodes as ...`. You could standardize on the same minimal types where appropriate to reduce casts. diff --git a/frontend/src/components/graph/NodeMenubar.tsx b/frontend/src/components/graph/NodeMenubar.tsx index e695f5c..460e072 100644 --- a/frontend/src/components/graph/NodeMenubar.tsx +++ b/frontend/src/components/graph/NodeMenubar.tsx @@ -1,3 +1,8 @@ +/** + * Per-node menubar (Node, Output, Data, Inputs, Insert). Content can be passed as props + * or resolved from the node type descriptor: when nodeMenuExtraContent is not provided, + * it is taken from getNodeType(nodeType)?.getNodeMenuExtraContent?.(nodeId, data). + */ import React, { useCallback, useContext, useMemo } from 'react' import FlowContext from '@/lib/graph/flowContext' import { getNodeType } from '@/lib/graph/nodeRegistry' diff --git a/frontend/src/components/nodes/config/descriptor.tsx b/frontend/src/components/nodes/config/descriptor.tsx index 7319243..348e7f4 100644 --- a/frontend/src/components/nodes/config/descriptor.tsx +++ b/frontend/src/components/nodes/config/descriptor.tsx @@ -13,7 +13,7 @@ import { import { getResolvedContentForConfig } from './renderingLogic' import type { NodeTypeDescriptor } from '@/lib/graph/nodeRegistry' import ConfigNode from './ConfigNode' -import { createImageExportHandlers, type OutputMenuContext } from '@/components/nodes/render/outputMenuRegistry' +import { createImageExportHandlers, type OutputMenuContext } from '@/components/nodes/render/outputMenuHandlers' const ICON_CLASS = 'mr-2 h-4 w-4' diff --git a/frontend/src/components/nodes/config/renderingLogic.ts b/frontend/src/components/nodes/config/renderingLogic.ts index f492bcd..9ac4145 100644 --- a/frontend/src/components/nodes/config/renderingLogic.ts +++ b/frontend/src/components/nodes/config/renderingLogic.ts @@ -7,43 +7,11 @@ import nunjucks from 'nunjucks' import type { ResolvedContentResult, SourceRenderingLogicContext } from '@/lib/graph/rendering' import { getConfigContent, getConfigType, getConfigTypeId, type ConfigTypeId } from '@/lib/graph/rendering' +import { isReachable, resolveExtendsRef, getTemplateRefs } from '@/lib/graph/templateRefs' type Node = { id: string; type?: string; data?: unknown } type Edge = { id: string; source: string; target: string } -function isReachable(edges: Edge[], startId: string, targetId: string): boolean { - const q: string[] = [startId] - const seen = new Set([startId]) - while (q.length) { - const cur = q.shift()! - if (cur === targetId) return true - for (const e of edges) { - if (e.source === cur && !seen.has(e.target)) { - seen.add(e.target) - q.push(e.target) - } - } - } - return false -} - -function resolveExtendsRef(nodes: Node[], name: string): string { - const refName = name.replace(/\.(puml|html)$/, '').trim() - return nodes.find((n) => n.id === refName || (n.data as Record)?.title === refName)?.id ?? refName -} - -function getTemplateRefs(content: string): string[] { - const refs: string[] = [] - const extendMatch = content.match(/\{\%\s*extends\s+["']([^"']+)["']\s*\%\}/) - if (extendMatch) refs.push(extendMatch[1].trim()) - const includeRegex = /\{\%\s*include\s+["']([^"']+)["']\s*\%\}/g - let m - while ((m = includeRegex.exec(content)) !== null) refs.push(m[1].trim()) - const importRegex = /\{\%\s*import\s+["']([^"']+)["']\s+as\s+\w+\s*\%\}/g - while ((m = importRegex.exec(content)) !== null) refs.push(m[1].trim()) - return refs -} - export async function getResolvedContentForConfig(context: SourceRenderingLogicContext): Promise { const { nodes, edges, sourceNodeId, renderNodeId } = context const srcId = sourceNodeId @@ -56,11 +24,11 @@ export async function getResolvedContentForConfig(context: SourceRenderingLogicC const configIdsUsed = new Set() const addConfigAndRefs = (templateName: string, visited = new Set()) => { - const refId = resolveExtendsRef(nodes, templateName) + const refId = resolveExtendsRef(nodes as { id: string; data?: unknown }[], templateName) if (visited.has(refId)) throw new Error(`Circular reference detected: ${templateName}`) const node = nodes.find((n) => n.id === refId && n.type === 'config') if (!node) throw new Error(`Config not found: ${templateName}`) - if (refId !== srcId && !isReachable(edges, refId, id)) + if (refId !== srcId && !isReachable(edges as { source: string; target: string }[], refId, id)) throw new Error(`Referenced config not connected to renderer: ${templateName}`) visited.add(refId) configIdsUsed.add(refId) @@ -72,10 +40,10 @@ export async function getResolvedContentForConfig(context: SourceRenderingLogicC const configLoader = { getSource: (name: string): { src: string; path: string } | null => { - const refId = resolveExtendsRef(nodes, name) + const refId = resolveExtendsRef(nodes as { id: string; data?: unknown }[], name) const node = nodes.find((n) => n.id === refId && n.type === 'config') if (!node) return null - if (refId !== srcId && !isReachable(edges, refId, id)) + if (refId !== srcId && !isReachable(edges as { source: string; target: string }[], refId, id)) throw new Error(`Referenced config not connected to renderer: ${name}`) return { src: getConfigContent((node.data ?? undefined) as Record | undefined), diff --git a/frontend/src/components/nodes/render/index.ts b/frontend/src/components/nodes/render/index.ts index 64053ee..d8e6a7f 100644 --- a/frontend/src/components/nodes/render/index.ts +++ b/frontend/src/components/nodes/render/index.ts @@ -1,4 +1,4 @@ export { default as RenderingNode, type RenderingNodeData } from './RenderingNode' export { getRenderNodeDescriptor } from './descriptor' -export { createImageExportHandlers } from './outputMenuRegistry' -export type { OutputMenuContext } from './outputMenuRegistry' +export { createImageExportHandlers } from './outputMenuHandlers' +export type { OutputMenuContext } from './outputMenuHandlers' diff --git a/frontend/src/components/nodes/render/outputMenuRegistry.tsx b/frontend/src/components/nodes/render/outputMenuHandlers.tsx similarity index 100% rename from frontend/src/components/nodes/render/outputMenuRegistry.tsx rename to frontend/src/components/nodes/render/outputMenuHandlers.tsx diff --git a/frontend/src/components/nodes/render/useRenderingNodeState.ts b/frontend/src/components/nodes/render/useRenderingNodeState.ts index d500c39..0273544 100644 --- a/frontend/src/components/nodes/render/useRenderingNodeState.ts +++ b/frontend/src/components/nodes/render/useRenderingNodeState.ts @@ -20,6 +20,7 @@ import { processSvgDisplay, stripTemplateSyntax, } from '@/lib/graph/rendering' +import { isReachable, resolveExtendsRef, getTemplateRefs } from '@/lib/graph/templateRefs' import type { ConfigTypeId, SourceRenderingLogicContext } from '@/lib/graph/rendering' export type RenderingNodeData = { @@ -136,54 +137,22 @@ export function useRenderingNodeState( : '' const connectedNodeIds = useMemo(() => { + const edgeList = edges as { source: string; target: string }[] + const nodeList = nodes as { id: string; type?: string; data?: unknown }[] const out = new Set() - const isReachable = (startId: string, targetId: string) => { - const q: string[] = [startId] - const seen = new Set([startId]) - while (q.length) { - const cur = q.shift()! - if (cur === targetId) return true - for (const e of edges as { source: string; target: string }[]) { - if (e.source === cur && !seen.has(e.target)) { - seen.add(e.target) - q.push(e.target) - } - } - } - return false - } - const resolveRef = (name: string) => { - const refName = name.replace(/\.(puml|html)$/, '').trim() - return (nodes as { id: string; data?: { title?: string } }[]).find( - (n) => n.id === refName || n.data?.title === refName - )?.id ?? refName - } - const getTemplateRefs = (content: string): string[] => { - const refs: string[] = [] - const extendMatch = content.match(/\{\%\s*extends\s+["']([^"']+)["']\s*\%\}/) - if (extendMatch) refs.push(extendMatch[1].trim()) - const includeRegex = /\{\%\s*include\s+["']([^"']+)["']\s*\%\}/g - let m - while ((m = includeRegex.exec(content)) !== null) refs.push(m[1].trim()) - const importRegex = /\{\%\s*import\s+["']([^"']+)["']\s+as\s+\w+\s*\%\}/g - while ((m = importRegex.exec(content)) !== null) refs.push(m[1].trim()) - return refs - } const addConfigRefs = (nodeId: string, visited: Set) => { if (visited.has(nodeId)) return - const node = (nodes as { id: string; type?: string; data?: unknown }[]).find( - (n) => n.id === nodeId && n.type === 'config' - ) + const node = nodeList.find((n) => n.id === nodeId && n.type === 'config') if (!node) return visited.add(nodeId) out.add(nodeId) const content = getConfigContent((node.data ?? undefined) as Record | undefined) for (const ref of getTemplateRefs(content)) { - const refId = resolveRef(ref) + const refId = resolveExtendsRef(nodeList, ref) if ( refId && - (nodes as { id: string; type?: string }[]).some((n) => n.id === refId && n.type === 'config') && - isReachable(refId, id) + nodeList.some((n) => n.id === refId && n.type === 'config') && + isReachable(edgeList, refId, id) ) { addConfigRefs(refId, visited) } @@ -191,11 +160,11 @@ export function useRenderingNodeState( } const configVisited = new Set() for (const nid of incomingIds) { - const node = (nodes as { id: string; type?: string }[]).find((n) => n.id === nid) + const node = nodeList.find((n) => n.id === nid) if (node?.type === 'config') addConfigRefs(nid, configVisited) else out.add(nid) } - for (const e of edges as { source: string; target: string }[]) { + for (const e of edgeList) { if (out.has(e.target)) out.add(e.source) } return out diff --git a/frontend/src/lib/graph/nodeTypes.ts b/frontend/src/lib/graph/nodeTypes.ts index b49a2d2..d07ffa6 100644 --- a/frontend/src/lib/graph/nodeTypes.ts +++ b/frontend/src/lib/graph/nodeTypes.ts @@ -1,5 +1,7 @@ /** - * Central node and edge types for the app. Use AppNode / AppEdge in graph state and context. + * React Flow node/edge types for the app (graph state and context). AppNode / AppEdge + * describe the shape of nodes and edges; the node type *id* (e.g. 'config', 'render') + * lives in nodeRegistry and is stored as node.type. */ import type { Node, Edge } from '@xyflow/react' diff --git a/frontend/src/lib/graph/rendering.ts b/frontend/src/lib/graph/rendering.ts index 55bbc1e..a32a3dd 100644 --- a/frontend/src/lib/graph/rendering.ts +++ b/frontend/src/lib/graph/rendering.ts @@ -13,15 +13,20 @@ * * 3. **Display** — The Rendering node picks a view (image viewport vs markdown content) * from `outputType` on the renderer and shows the result. Output menu items (e.g. Export) - * come from the source/renderer via `outputMenuDescriptor`. + * come from the source node descriptor's getOutputMenuContent (see NodeTypeDescriptor). * * ## Implementing a new source - * - Implement `IRenderingSource` (resolve + optional output menu). - * - Register with `registerSourceRenderingLogic(nodeType, logic)`. + * - Implement source rendering logic (resolve step) and set it on the node descriptor + * via .sourceRenderingLogic(). Optionally set .outputMenuContent() for the Output menu. + * - The descriptor is registered via registerNodeType(); source logic is registered automatically. * * ## Implementing a new output type * - Add a config type (or equivalent) fulfilling `IOutputTypeRenderer`. * - Register in CONFIG_TYPES and use `getConfigType(id)` in the resolve step. + * + * ## Shared helpers + * - templateRefs.ts: isReachable, resolveExtendsRef, getTemplateRefs (Nunjucks extends/include/import). + * Used by config resolve and the rendering node hook so template/reachability logic stays in one place. */ import type { @@ -60,8 +65,7 @@ export interface IResolveResult { /** * Contract for a node type that can feed the Rendering node (config, agent, etc.). - * Register via registerSourceRenderingLogic(nodeType, logic). - * Output menu content is registered per output type in the frontend (outputMenuRegistry). + * Set on the node descriptor via .sourceRenderingLogic(); output menu via .outputMenuContent(). */ export interface IRenderingSource { /** When to re-run: 'auto' on upstream changes, 'manual' only on Run. */ diff --git a/frontend/src/lib/graph/sourceRenderingLogic.ts b/frontend/src/lib/graph/sourceRenderingLogic.ts index d974f45..980ed1c 100644 --- a/frontend/src/lib/graph/sourceRenderingLogic.ts +++ b/frontend/src/lib/graph/sourceRenderingLogic.ts @@ -30,7 +30,7 @@ export type ResolvedContentResult = { reasoning?: string } -/** Source logic implementation. Implements {@link IRenderingSource}. Output menu content is provided per output type via the frontend registry (see outputMenuRegistry). */ +/** Source logic implementation. Implements {@link IRenderingSource}. Output menu is provided by the node descriptor's getOutputMenuContent. */ export type SourceRenderingLogic = { defaultUpdateMode: 'auto' | 'manual' getResolvedContent: (context: SourceRenderingLogicContext) => Promise diff --git a/frontend/src/lib/graph/templateRefs.ts b/frontend/src/lib/graph/templateRefs.ts new file mode 100644 index 0000000..cb82126 --- /dev/null +++ b/frontend/src/lib/graph/templateRefs.ts @@ -0,0 +1,54 @@ +/** + * Shared helpers for Nunjucks template references (extends/include/import) and + * graph reachability. Used by config resolve logic and the rendering node hook + * so template parsing and "connected configs" stay in one place. + */ + +export type EdgeLike = { source: string; target: string } +export type NodeLike = { id: string; data?: unknown } + +/** BFS: is target reachable from start following directed edges? */ +export function isReachable( + edges: EdgeLike[], + startId: string, + targetId: string +): boolean { + const q: string[] = [startId] + const seen = new Set([startId]) + while (q.length) { + const cur = q.shift()! + if (cur === targetId) return true + for (const e of edges) { + if (e.source === cur && !seen.has(e.target)) { + seen.add(e.target) + q.push(e.target) + } + } + } + return false +} + +/** Resolve a template name to a node id (by id or by data.title). */ +export function resolveExtendsRef(nodes: NodeLike[], name: string): string { + const refName = name.replace(/\.(puml|html)$/, '').trim() + return ( + nodes.find( + (n) => + n.id === refName || + (n.data as Record)?.title === refName + )?.id ?? refName + ) +} + +/** Extract template refs from content: extends "x", include "y", import "z" as ... */ +export function getTemplateRefs(content: string): string[] { + const refs: string[] = [] + const extendMatch = content.match(/\{\%\s*extends\s+["']([^"']+)["']\s*\%\}/) + if (extendMatch) refs.push(extendMatch[1].trim()) + const includeRegex = /\{\%\s*include\s+["']([^"']+)["']\s*\%\}/g + let m + while ((m = includeRegex.exec(content)) !== null) refs.push(m[1].trim()) + const importRegex = /\{\%\s*import\s+["']([^"']+)["']\s+as\s+\w+\s*\%\}/g + while ((m = importRegex.exec(content)) !== null) refs.push(m[1].trim()) + return refs +}