From 68f94d4970b2d15b9a6b599e9ae0e3772a5a9137 Mon Sep 17 00:00:00 2001 From: Alan Lail Date: Thu, 17 Sep 2026 15:16:30 -0400 Subject: [PATCH] Further optimizations of data tree --- .../src/domain/framework/treeDerivation.ts | 28 +- apps/editor/src/ui/editor/EditorCanvas.tsx | 260 +++++++++++++----- .../src/ui/editor/state/EditorContext.tsx | 30 +- .../src/ui/editor/state/editorReducer.test.ts | 82 ++++++ .../src/ui/editor/state/editorReducer.ts | 54 +++- .../ui/editor/treePanel/FrameworkTreeItem.tsx | 50 +++- .../ui/editor/treePanel/FrameworkTreeList.tsx | 5 + .../src/ui/editor/treePanel/TreePanelView.tsx | 48 +++- .../treePanel/buildFrameworkTree.test.ts | 66 ++--- 9 files changed, 466 insertions(+), 157 deletions(-) diff --git a/apps/editor/src/domain/framework/treeDerivation.ts b/apps/editor/src/domain/framework/treeDerivation.ts index 53571d8..c70504f 100644 --- a/apps/editor/src/domain/framework/treeDerivation.ts +++ b/apps/editor/src/domain/framework/treeDerivation.ts @@ -1,8 +1,5 @@ -import type { CFItem } from '@/domain/case/types' - export interface FrameworkTreeNode { id: string - cfItem: CFItem children: FrameworkTreeNode[] depth: number } @@ -17,17 +14,19 @@ export type FrameworkEdgeRecord = { /** * Pure domain function — no React Flow imports. * - * @param cfItems All CFItems in the framework. - * @param edges Parent→child relationships (item-to-item only, no framework-root edges). + * Builds only the tree SHAPE (id/children/depth) from parent→child edges. + * Item content (CFItem) is intentionally NOT embedded here — callers look it + * up separately (e.g. by id, from a Map) so that editing an item's own field + * data doesn't change this shape and force a full tree rebuild; only actual + * structural changes (items added/removed/reparented) should. + * + * @param edges Parent→child relationships (item-to-item only, no framework-root edges). * @param rootItemIds Top-level item IDs in sequence order (pre-sorted by the caller). */ export function buildFrameworkTree( - cfItems: CFItem[], edges: FrameworkEdgeRecord[], rootItemIds: string[], ): FrameworkTreeNode[] { - const itemById = new Map(cfItems.map((item) => [item.identifier, item])) - // Build parent → [{childId, seq}] map const childrenOf = new Map() for (const edge of edges) { @@ -41,17 +40,18 @@ export function buildFrameworkTree( entry.sort((a, b) => a.seq - b.seq) } - function buildNode(id: string, depth: number): FrameworkTreeNode | null { - const item = itemById.get(id) - if (!item) return null + function buildNode(id: string, depth: number, visiting: Set): FrameworkTreeNode | null { + if (visiting.has(id)) return null // cycle guard: malformed/cyclic hierarchical data + visiting.add(id) const childEntries = childrenOf.get(id) ?? [] const children = childEntries - .map((e) => buildNode(e.childId, depth + 1)) + .map((e) => buildNode(e.childId, depth + 1, visiting)) .filter((n): n is FrameworkTreeNode => n !== null) - return { id, cfItem: item, children, depth } + visiting.delete(id) + return { id, children, depth } } return rootItemIds - .map((id) => buildNode(id, 0)) + .map((id) => buildNode(id, 0, new Set())) .filter((n): n is FrameworkTreeNode => n !== null) } diff --git a/apps/editor/src/ui/editor/EditorCanvas.tsx b/apps/editor/src/ui/editor/EditorCanvas.tsx index fc55f91..acd0299 100644 --- a/apps/editor/src/ui/editor/EditorCanvas.tsx +++ b/apps/editor/src/ui/editor/EditorCanvas.tsx @@ -1,4 +1,4 @@ -import { useCallback, useEffect, useMemo, useRef, useState, type MouseEvent as ReactMouseEvent, type PointerEvent as ReactPointerEvent } from 'react' +import { memo, useCallback, useEffect, useMemo, useRef, useState, type MouseEvent as ReactMouseEvent, type PointerEvent as ReactPointerEvent } from 'react' import type { ReactFlowInstance, Connection, Edge, NodeChange, EdgeChange } from '@xyflow/react' import type { OnBeforeDelete } from '@xyflow/react' import type { OnSelectionChangeFunc } from '@xyflow/react' @@ -28,6 +28,135 @@ import { absolutizeCaseUris, frameworkToCfPackage, toOpenCaseFormat } from '@/ap import type { Framework } from '@/domain/framework/model/types' import { hasFrameworkDataChanged } from '@/domain/framework/hasFrameworkDataChanged' +// ── Stable ReactFlow config (module scope — never recreated) ────────────── +// +// A fresh object/array/function literal passed as a prop to on +// every EditorCanvas render defeats any attempt to keep it from doing work +// while hidden behind Tree View: CPU profiling showed React Flow's internal +// store (setState/shallow-equality selectors) re-syncing on every keystroke +// even when node/edge DATA was unchanged, because other props (these) were +// still fresh references each render. +const REACT_FLOW_DEFAULT_EDGE_OPTIONS = { + interactionWidth: 20, + style: { strokeWidth: 1.5, stroke: '#94a3b8' }, + focusable: true, + reconnectable: true, +} +const REACT_FLOW_PRO_OPTIONS = { hideAttribution: true } +const REACT_FLOW_BACKGROUND_STYLE = { backgroundColor: '#f0f0f2' } +const minimapNodeColor = (node: CaseEditorNodeType) => (node.selected ? '#8b5cf6' : '#e2e8f0') // violet-500 if selected, slate-200 otherwise +const minimapNodeStrokeColor = (node: CaseEditorNodeType) => (node.selected ? '#7c3aed' : '#cbd5e1') // violet-600 if selected, slate-300 otherwise + +type ReactFlowGraphProps = { + wrapRef: React.RefObject + visible: boolean + nodes: CaseEditorNodeType[] + edges: CaseEditorEdge[] + onNodesChange: (changes: NodeChange[]) => void + onEdgesChange: (changes: EdgeChange[]) => void + onConnect: (connection: Connection) => void + onNodeClick: (event: ReactMouseEvent, node: CaseEditorNodeType) => void + onNodeDragStart: (event: ReactMouseEvent, node: CaseEditorNodeType) => void + onNodeDragStop: () => void + onEdgeClick: (event: ReactMouseEvent, edge: Edge) => void + onPaneClick: (event: ReactMouseEvent) => void + isValidConnection: (connection: Connection) => boolean + onSelectionChange: OnSelectionChangeFunc + onBeforeDelete: OnBeforeDelete + nodesDraggable: boolean + onReconnectStart: () => void + onReconnect: (oldEdge: Edge, newConnection: Connection) => void + onReconnectEnd: (_: unknown, edge: Edge) => void + onInit: (instance: ReactFlowInstance) => void + onPointerDownCapture: (event: ReactPointerEvent) => void +} + +/** + * Isolated in its own `React.memo`'d component (rather than inline JSX in + * EditorCanvas) so that when every prop here is referentially stable — + * which is the case while Tree View is active, since `nodes`/`edges` are + * frozen and every handler is `useCallback`'d — React skips calling this + * component's render function entirely, instead of merely receiving + * unchanged props. That's what actually stops React Flow's internal effects + * from re-running on every keystroke while the canvas is invisible. + */ +const ReactFlowGraph = memo(function ReactFlowGraph({ + wrapRef, + visible, + nodes, + edges, + onNodesChange, + onEdgesChange, + onConnect, + onNodeClick, + onNodeDragStart, + onNodeDragStop, + onEdgeClick, + onPaneClick, + isValidConnection, + onSelectionChange, + onBeforeDelete, + nodesDraggable, + onReconnectStart, + onReconnect, + onReconnectEnd, + onInit, + onPointerDownCapture, +}: ReactFlowGraphProps) { + return ( +
+ + nodes={nodes} + edges={edges} + onNodesChange={onNodesChange} + onEdgesChange={onEdgesChange} + onConnect={onConnect} + onNodeClick={onNodeClick} + onNodeDragStart={onNodeDragStart} + onNodeDragStop={onNodeDragStop} + onEdgeClick={onEdgeClick} + onPaneClick={onPaneClick} + isValidConnection={isValidConnection} + onSelectionChange={onSelectionChange} + onBeforeDelete={onBeforeDelete} + selectionMode={SelectionMode.Full} + multiSelectionKeyCode="Meta" + selectionOnDrag={false} + selectionKeyCode="Shift" + panOnDrag + nodesDraggable={nodesDraggable} + nodeTypes={nodeTypes} + edgeTypes={edgeTypes} + edgesFocusable + elevateEdgesOnSelect + edgesReconnectable + onReconnectStart={onReconnectStart} + onReconnect={onReconnect} + onReconnectEnd={onReconnectEnd} + connectOnClick={true} + connectionMode={ConnectionMode.Loose} + onlyRenderVisibleElements + defaultEdgeOptions={REACT_FLOW_DEFAULT_EDGE_OPTIONS} + proOptions={REACT_FLOW_PRO_OPTIONS} + onInit={onInit} + > + + + + +
+ ) +}) + type MirrorStatus = { isModifiedFromSource?: boolean; sourcePackageURI?: string } type EditorCanvasProps = { @@ -390,6 +519,31 @@ export default function EditorCanvas({ onBack, onSaveToServer, isPublishedToOpen return result }, [editorEdges, settings.edgeType, effectiveGroupingFilter]) + // React Flow stays MOUNTED (just CSS-hidden) while Tree View is active, so + // that pan/zoom/selection state survives switching views. But feeding it a + // fresh `nodes`/`edges` array reference on every keystroke — even while + // invisible — makes it redo internal store diffing across the whole graph + // for nothing (confirmed via CPU profile: React Flow's internal selectors/ + // shallow-equality checks dominate keystroke cost at ~7,500 nodes). Freeze + // the props actually delivered to while hidden, and only catch + // up to the latest data the moment the canvas becomes visible again. + const frozenCanvasGraphRef = useRef<{ nodes: CaseEditorNodeType[]; edges: CaseEditorEdge[] }>({ + nodes: nodesWithCallbacks, + edges: edgesWithType, + }) + if (activeView !== 'tree') { + frozenCanvasGraphRef.current = { nodes: nodesWithCallbacks, edges: edgesWithType } + } + const canvasNodes = activeView === 'tree' ? frozenCanvasGraphRef.current.nodes : nodesWithCallbacks + const canvasEdges = activeView === 'tree' ? frozenCanvasGraphRef.current.edges : edgesWithType + + // Stable identity (see comment above) — an inline arrow function here would + // itself defeat the freeze the same way defaultEdgeOptions/proOptions did. + const onReactFlowInit = useCallback((instance: ReactFlowInstance) => { + reactFlowRef.current = instance + setRfReady(true) + }, []) + // Validate connections - prevent framework-to-framework connections const nodesWithCallbacksRef = useRef(nodesWithCallbacks) nodesWithCallbacksRef.current = nodesWithCallbacks @@ -635,7 +789,10 @@ export default function EditorCanvas({ onBack, onSaveToServer, isPublishedToOpen const flowX = (pointer.clientX - rect.left - viewport.x) / viewport.zoom const flowY = (pointer.clientY - rect.top - viewport.y) / viewport.zoom const hitSelectedNodeId = selectedNodeIds.find((id) => { - const node = nodes.find((n) => n.id === id) + // Read via graphRef, not the closed-over `nodes`, so this callback's + // identity doesn't change on every keystroke (nodes' reference + // changes on every dispatch, even ones that don't touch positions). + const node = graphRef.current.nodes.find((n) => n.id === id) if (!node) return false const anyNode = node as unknown as { measured?: { width?: number; height?: number } @@ -716,7 +873,7 @@ export default function EditorCanvas({ onBack, onSaveToServer, isPublishedToOpen onNodesChange(changes) logSelectionDebug('onNodesChange/forwarded', { changeCount: changes.length }) - }, [logSelectionDebug, nodes, onNodesChange, selectedNodeIds]) + }, [logSelectionDebug, onNodesChange, selectedNodeIds]) const onEdgesChangeWithSelectionGuard = useCallback((changes: EdgeChange[]) => { const suppressEcho = suppressSelectEchoRef.current @@ -893,17 +1050,23 @@ export default function EditorCanvas({ onBack, onSaveToServer, isPublishedToOpen const onBeforeDelete: OnBeforeDelete = useCallback( async ({ nodes, edges: deletedEdges }) => { + // Read via refs, not the closed-over nodesWithCallbacks/editorEdges, so + // this callback's identity doesn't change on every keystroke (both + // change reference on every dispatch, even ones that don't affect + // what's deletable) — see graphRef/nodesWithCallbacksRef above. + const allNodes = nodesWithCallbacksRef.current + const allEdges = graphRef.current.edges const includesFramework = nodes.some((n) => n.type === 'caseFrameworkNode') - const nodeIds = includesFramework ? nodesWithCallbacks.map((n) => n.id) : nodes.map((n) => n.id) - const edgeIds = includesFramework ? editorEdges.map((e) => e.id) : deletedEdges.map((e) => e.id) + const nodeIds = includesFramework ? allNodes.map((n) => n.id) : nodes.map((n) => n.id) + const edgeIds = includesFramework ? allEdges.map((e) => e.id) : deletedEdges.map((e) => e.id) const nodeIdSet = new Set(nodeIds) const deletedItemIdSet = new Set( - (includesFramework ? nodesWithCallbacks : nodes).filter((n) => n.type === 'caseItemNode').map((n) => n.id), + (includesFramework ? allNodes : nodes).filter((n) => n.type === 'caseItemNode').map((n) => n.id), ) - const childItemCount = nodesWithCallbacks.filter( + const childItemCount = allNodes.filter( (n) => n.type === 'caseItemNode' && !nodeIdSet.has(n.id) && @@ -925,7 +1088,7 @@ export default function EditorCanvas({ onBack, onSaveToServer, isPublishedToOpen }) }) }, - [nodesWithCallbacks, editorEdges], + [], ) const closeActionDialog = useCallback(() => { @@ -1300,64 +1463,29 @@ export default function EditorCanvas({ onBack, onSaveToServer, isPublishedToOpen ) : null} -
- - nodes={nodesWithCallbacks} - edges={edgesWithType} - onNodesChange={onNodesChangeWithSelectionGuard} - onEdgesChange={onEdgesChangeWithSelectionGuard} - onConnect={onConnect} - onNodeClick={onNodeClick} - onNodeDragStart={onNodeDragStart} - onNodeDragStop={onNodeDragStop} - onEdgeClick={onEdgeClick} - onPaneClick={onPaneClick} - isValidConnection={isValidConnection} - onSelectionChange={onSelectionChangeWithPan} - onBeforeDelete={onBeforeDelete} - selectionMode={SelectionMode.Full} - multiSelectionKeyCode="Meta" - selectionOnDrag={false} - selectionKeyCode="Shift" - panOnDrag - nodesDraggable={!shiftHeldForInteractions} - nodeTypes={nodeTypes} - edgeTypes={edgeTypes} - edgesFocusable - elevateEdgesOnSelect - edgesReconnectable - onReconnectStart={onReconnectStart} - onReconnect={onReconnect} - onReconnectEnd={onReconnectEnd} - connectOnClick={true} - connectionMode={ConnectionMode.Loose} - onlyRenderVisibleElements - defaultEdgeOptions={{ - interactionWidth: 20, - style: { strokeWidth: 1.5, stroke: '#94a3b8' }, - focusable: true, - reconnectable: true, - }} - proOptions={{ hideAttribution: true }} - onInit={(instance) => { - reactFlowRef.current = instance as unknown as ReactFlowInstance - setRfReady(true) - }} - > - - - (node.selected ? '#8b5cf6' : '#e2e8f0')} // violet-500 if selected, slate-200 otherwise - nodeStrokeColor={(node) => (node.selected ? '#7c3aed' : '#cbd5e1')} // violet-600 if selected, slate-300 otherwise - maskColor="rgba(240, 240, 245, 0.7)" - /> - -
+ state.nodes.filter(isItemNode).map((n) => n.data.cfItem), - [state.nodes], - ) + // Cached derivation (same pattern as `nodesWithCallbacks` below): if every + // item node is referentially unchanged from the previous render (e.g. a + // dispatch only touched the framework node or selection state), reuse the + // previous `cfItems` array instead of allocating a new one. This keeps + // `cfItems`'s identity stable across non-item edits, which matters because + // downstream consumers (Tree Panel's `buildFrameworkTree`) key expensive + // recomputation off this array's reference. + const cfItemsCacheRef = useRef<{ inputs: CaseItemNodeType[]; output: CFItem[] }>({ inputs: [], output: [] }) + const cfItems = useMemo(() => { + const itemNodes = state.nodes.filter(isItemNode) + const prev = cfItemsCacheRef.current + if (itemNodes.length === prev.inputs.length && itemNodes.every((n, i) => n === prev.inputs[i])) { + return prev.output + } + const output = itemNodes.map((n) => n.data.cfItem) + cfItemsCacheRef.current = { inputs: itemNodes, output } + return output + }, [state.nodes]) const frameworkEdges = useMemo( (): FrameworkEdgeRecord[] => state.edges - .filter((e) => !e.data?.isFrameworkRootConnection) + // Only hierarchical item-to-item edges (isChildOf/isPartOf) represent + // parent/child structure — non-hierarchical associations (isRelatedTo, + // precedes, etc.) must NOT be treated as tree edges here, or a + // reciprocal/cross-type association can introduce a cycle into what + // buildFrameworkTree assumes is a DAG rooted at the framework node + // (mirrors the isHierarchical filtering in nodeGeometry.ts buildAdjacency). + .filter((e) => !e.data?.isFrameworkRootConnection && e.data?.isHierarchical) .map((e) => ({ parentId: e.source, childId: e.target, diff --git a/apps/editor/src/ui/editor/state/editorReducer.test.ts b/apps/editor/src/ui/editor/state/editorReducer.test.ts index d837e6e..47dd677 100644 --- a/apps/editor/src/ui/editor/state/editorReducer.test.ts +++ b/apps/editor/src/ui/editor/state/editorReducer.test.ts @@ -346,6 +346,88 @@ describe('graph/delete', () => { }) }) +// ── Referential identity (perf regression guards) ────────────────────── +// +// These guard against re-introducing full-array reallocation for actions +// that should only touch a specific node/edge — see POR-736 phase 2. +// Downstream memoization (EditorContext's `nodesWithCallbacks`, Tree Panel's +// `cfItemsById`) relies on untouched entries keeping their exact object +// reference across a dispatch. + +describe('referential identity', () => { + it('node/updateData on one item leaves other nodes untouched', () => { + const state = makeState() + const otherNode = state.nodes.find((n) => n.id === 'item-2') + const next = editorReducer(state, { + type: 'node/updateData', + nodeId: 'item-1', + patch: { cfItem: { fullStatement: 'Updated' } }, + }) + expect(next.nodes.find((n) => n.id === 'item-2')).toBe(otherNode) + }) + + it('node/addChild only reallocates the previously-selected node and the new one', () => { + const state = makeState({ selectedNodeId: 'item-2', selectedNodeIds: ['item-2'] }) + const untouchedItem = state.nodes.find((n) => n.id === 'item-1') + const untouchedEdgeToItem1 = state.edges.find((e) => e.target === 'item-1') + + const next = editorReducer(state, { + type: 'node/addChild', + parentId: 'item-1', + childId: 'child-1', + cfItem: { identifier: 'child-1', uri: '', fullStatement: 'Child', lastChangeDateTime: '' }, + }) + + expect(next.nodes.find((n) => n.id === 'item-1')).toBe(untouchedItem) + expect(next.edges.find((e) => e.target === 'item-1')).toBe(untouchedEdgeToItem1) + // The previously-selected node should be the only pre-existing node reallocated (selected: false) + const previouslySelected = next.nodes.find((n) => n.id === 'item-2') + expect(previouslySelected?.selected).toBe(false) + }) + + it('node/addDetachedItem leaves all existing nodes untouched except the previously-selected one', () => { + const state = makeState({ selectedNodeId: 'item-1', selectedNodeIds: ['item-1'] }) + const untouchedItem = state.nodes.find((n) => n.id === 'item-2') + + const next = editorReducer(state, { + type: 'node/addDetachedItem', + nodeId: 'detached-1', + cfItem: { identifier: 'detached-1', uri: '', fullStatement: 'Detached', lastChangeDateTime: '' }, + }) + + expect(next.nodes.find((n) => n.id === 'item-2')).toBe(untouchedItem) + }) + + it('node/addExternalFramework leaves all existing nodes untouched except the previously-selected one', () => { + const state = makeState({ selectedNodeId: 'item-1', selectedNodeIds: ['item-1'] }) + const untouchedItem = state.nodes.find((n) => n.id === 'item-2') + + const next = editorReducer(state, { + type: 'node/addExternalFramework', + nodeId: 'ext-1', + data: { title: 'External' }, + }) + + expect(next.nodes.find((n) => n.id === 'item-2')).toBe(untouchedItem) + }) + + it('graph/delete leaves untouched remaining nodes/edges referentially identical', () => { + const state = makeState() + const untouchedNode = state.nodes.find((n) => n.id === 'item-2') + const untouchedEdge = state.edges.find((e) => e.target === 'item-2') + + const next = editorReducer(state, { + type: 'graph/delete', + nodeIds: ['item-1'], + edgeIds: [], + reattachChildren: false, + }) + + expect(next.nodes.find((n) => n.id === 'item-2')).toBe(untouchedNode) + expect(next.edges.find((e) => e.target === 'item-2')).toBe(untouchedEdge) + }) +}) + // ── Default / unknown action ─────────────────────────────────────────── describe('unknown action', () => { diff --git a/apps/editor/src/ui/editor/state/editorReducer.ts b/apps/editor/src/ui/editor/state/editorReducer.ts index b1d881c..07b6f1c 100644 --- a/apps/editor/src/ui/editor/state/editorReducer.ts +++ b/apps/editor/src/ui/editor/state/editorReducer.ts @@ -70,6 +70,21 @@ export type Action = | { type: 'dirty/mark' } | { type: 'dirty/clear' } +// ── Helpers ──────────────────────────────────────────────────────────── + +/** + * Clear `selected` on only the given ids, leaving every other element's + * object reference untouched. Adding one node/edge shouldn't reallocate + * the entire array just to clear the previous selection — that breaks + * `nodesWithCallbacks`'s identity fast-path (EditorContext.tsx) for the + * whole graph on every "add item" action. + */ +function deselectExcept(items: T[], selectedIds: string[]): T[] { + if (!selectedIds.length) return items + const selectedSet = new Set(selectedIds) + return items.map((item) => (selectedSet.has(item.id) ? { ...item, selected: false } : item)) +} + // ── Reducer ──────────────────────────────────────────────────────────── export function editorReducer(state: EditorState, action: Action): EditorState { @@ -410,7 +425,7 @@ export function editorReducer(state: EditorState, action: Action): EditorState { className: WRAPPER_NODE_CLASS, } - const nextNodes = [...state.nodes.map((n) => ({ ...n, selected: false })), { ...childNode, selected: true }] + const nextNodes = [...deselectExcept(state.nodes, state.selectedNodeIds), { ...childNode, selected: true }] const handles = getClosestHandles(parent.position, parentSize, nextPosition, childSize) @@ -427,7 +442,7 @@ export function editorReducer(state: EditorState, action: Action): EditorState { } const nextEdges: CaseEditorEdge[] = [ - ...state.edges.map((e) => ({ ...e, selected: false })), + ...deselectExcept(state.edges, state.selectedEdgeIds), { id: `e_${action.parentId}_${childId}`, source: action.parentId, @@ -477,7 +492,7 @@ export function editorReducer(state: EditorState, action: Action): EditorState { className: WRAPPER_NODE_CLASS, } - const nextNodes = [...state.nodes.map((n) => ({ ...n, selected: false })), { ...newNode, selected: true }] + const nextNodes = [...deselectExcept(state.nodes, state.selectedNodeIds), { ...newNode, selected: true }] return { ...state, nodes: nextNodes, selectedNodeId: action.nodeId, selectedEdgeId: null, selectedNodeIds: [action.nodeId], selectedEdgeIds: [], dirty: true } } case 'node/addExternalFramework': { @@ -507,7 +522,7 @@ export function editorReducer(state: EditorState, action: Action): EditorState { className: WRAPPER_NODE_CLASS, } - const nextNodes = [...state.nodes.map((n) => ({ ...n, selected: false })), { ...newNode, selected: true }] + const nextNodes = [...deselectExcept(state.nodes, state.selectedNodeIds), { ...newNode, selected: true }] return { ...state, nodes: nextNodes, selectedNodeId: action.nodeId, selectedEdgeId: null, selectedNodeIds: [action.nodeId], selectedEdgeIds: [], dirty: true } } case 'graph/delete': { @@ -525,6 +540,17 @@ export function editorReducer(state: EditorState, action: Action): EditorState { const parentExists = new Set(remainingNodes.map((n) => n.id)) const reparentMap = new Map() + // Group remaining item nodes by their current parentId once, up + // front, instead of re-scanning all remaining nodes per deleted + // node (was O(deleted × remaining); now O(remaining + deleted)). + const childrenByParentId = new Map() + for (const n of remainingNodes) { + if (!isItemNode(n) || !n.data.parentId) continue + const list = childrenByParentId.get(n.data.parentId) ?? [] + list.push(n) + childrenByParentId.set(n.data.parentId, list) + } + for (const dn of deletedNodes) { if (!isItemNode(dn)) continue const parentId = dn.data.parentId @@ -532,10 +558,8 @@ export function editorReducer(state: EditorState, action: Action): EditorState { if (deleteNodeIds.has(parentId)) continue if (!parentExists.has(parentId)) continue - for (const n of remainingNodes) { - if (isItemNode(n) && n.data.parentId === dn.id) { - reparentMap.set(n.id, parentId) - } + for (const n of childrenByParentId.get(dn.id) ?? []) { + reparentMap.set(n.id, parentId) } } @@ -577,8 +601,18 @@ export function editorReducer(state: EditorState, action: Action): EditorState { selectedEdgeId, selectedNodeIds, selectedEdgeIds, - nodes: remainingNodes.map((n) => ({ ...n, selected: selectedNodeId ? n.id === selectedNodeId : false })), - edges: remainingEdges.map((e) => ({ ...e, selected: selectedEdgeId ? e.id === selectedEdgeId : false })) as CaseEditorEdge[], + // Only reallocate entries whose `selected` flag actually changes — + // touching every remaining node/edge on every delete defeats + // `nodesWithCallbacks`'s identity fast-path (EditorContext.tsx) for + // the whole graph. + nodes: remainingNodes.map((n) => { + const shouldBeSelected = selectedNodeId ? n.id === selectedNodeId : false + return Boolean(n.selected) === shouldBeSelected ? n : { ...n, selected: shouldBeSelected } + }), + edges: remainingEdges.map((e) => { + const shouldBeSelected = selectedEdgeId ? e.id === selectedEdgeId : false + return Boolean(e.selected) === shouldBeSelected ? e : { ...e, selected: shouldBeSelected } + }) as CaseEditorEdge[], dirty: true, } } diff --git a/apps/editor/src/ui/editor/treePanel/FrameworkTreeItem.tsx b/apps/editor/src/ui/editor/treePanel/FrameworkTreeItem.tsx index 95b18ab..4fb3cfb 100644 --- a/apps/editor/src/ui/editor/treePanel/FrameworkTreeItem.tsx +++ b/apps/editor/src/ui/editor/treePanel/FrameworkTreeItem.tsx @@ -1,9 +1,12 @@ -import { useState } from 'react' +import { memo, useState } from 'react' import { ChevronDown, ChevronRight, Link, Plus } from 'lucide-react' import type { FrameworkTreeNode } from '@/domain/framework/treeDerivation' +import type { CFItem } from '@/domain/case/types' type Props = { node: FrameworkTreeNode + /** Item content, looked up by id — kept separate from tree shape so content-only edits don't rebuild the tree. */ + cfItemsById: Map selectedId: string | null expandedIds: Set onToggleExpand: (_id: string) => void @@ -22,8 +25,9 @@ type Props = { onBadgeClick?: (_id: string, _e: React.MouseEvent) => void } -export default function FrameworkTreeItem({ +function FrameworkTreeItem({ node, + cfItemsById, selectedId, expandedIds, onToggleExpand, @@ -40,6 +44,7 @@ export default function FrameworkTreeItem({ onBadgeClick, }: Readonly) { const [hovered, setHovered] = useState(false) + const cfItem = cfItemsById.get(node.id) const isSelected = selectedId === node.id const isExpanded = expandedIds.has(node.id) const hasChildren = node.children.length > 0 @@ -112,13 +117,13 @@ export default function FrameworkTreeItem({
- {node.cfItem.humanCodingScheme ? ( + {cfItem?.humanCodingScheme ? ( - {node.cfItem.humanCodingScheme} + {cfItem.humanCodingScheme} ) : null} - {node.cfItem.identifier} + {node.id} {associationCount > 0 && (
- {node.cfItem.abbreviatedStatement?.trim() ? ( + {cfItem?.abbreviatedStatement?.trim() ? (

- {node.cfItem.abbreviatedStatement.trim()} + {cfItem.abbreviatedStatement.trim()}

) : null}

- {node.cfItem.fullStatement} + {cfItem?.fullStatement ?? '(item not found)'}

{hasChildren && ( @@ -170,6 +175,7 @@ export default function FrameworkTreeItem({ ) } + +// `node` (shape) is already stable across content-only edits (see +// treeDerivation.ts), but `cfItemsById`'s Map reference changes whenever ANY +// item's content changes, not just this row's — so the default shallow +// `memo` comparison would still re-render every row on every edit. Compare +// this row's own looked-up item instead of the whole Map reference. +function areEqual(prev: Readonly, next: Readonly): boolean { + return ( + prev.node === next.node && + prev.selectedId === next.selectedId && + prev.expandedIds === next.expandedIds && + prev.onToggleExpand === next.onToggleExpand && + prev.onSelect === next.onSelect && + prev.onAddChild === next.onAddChild && + prev.isDraggable === next.isDraggable && + prev.onDragStart === next.onDragStart && + prev.isDropTarget === next.isDropTarget && + prev.onDragOver === next.onDragOver && + prev.onDragLeave === next.onDragLeave && + prev.onDrop === next.onDrop && + prev.dragOverItemId === next.dragOverItemId && + prev.associationCounts === next.associationCounts && + prev.onBadgeClick === next.onBadgeClick && + prev.cfItemsById.get(next.node.id) === next.cfItemsById.get(next.node.id) + ) +} + +export default memo(FrameworkTreeItem, areEqual) diff --git a/apps/editor/src/ui/editor/treePanel/FrameworkTreeList.tsx b/apps/editor/src/ui/editor/treePanel/FrameworkTreeList.tsx index 51ebc26..cc4b2a5 100644 --- a/apps/editor/src/ui/editor/treePanel/FrameworkTreeList.tsx +++ b/apps/editor/src/ui/editor/treePanel/FrameworkTreeList.tsx @@ -2,6 +2,7 @@ import { Plus } from 'lucide-react' import { Button } from '@/ui/shared/components/ui/button' import FrameworkTreeItem from './FrameworkTreeItem' import type { FrameworkTreeNode } from '@/domain/framework/treeDerivation' +import type { CFItem } from '@/domain/case/types' type Props = { title: string @@ -9,6 +10,8 @@ type Props = { publisher?: string frameworkNodeId: string | null roots: FrameworkTreeNode[] + /** Item content, looked up by id — kept separate from tree shape so content-only edits don't rebuild `roots`. */ + cfItemsById: Map selectedId: string | null expandedIds: Set onToggleExpand: (_id: string) => void @@ -41,6 +44,7 @@ export default function FrameworkTreeList({ publisher, frameworkNodeId, roots, + cfItemsById, selectedId, expandedIds, onToggleExpand, @@ -113,6 +117,7 @@ export default function FrameworkTreeList({ { /* cursor hint only */ } + const ALIGNMENT_ASSOCIATION_TYPES: Array<{ value: string; label: string }> = [ { value: 'exactMatchOf', label: 'Exact Match Of' }, { value: 'isRelatedTo', label: 'Is Related To' }, @@ -244,9 +248,17 @@ export default function TreePanelView({ availableFrameworks = [], serverFramewor // ── Tree data ── + // Tree SHAPE — depends only on structure (edges/root order), never on item + // content, so editing an item's own fields doesn't force a rebuild of the + // whole tree. Content is looked up separately via `cfItemsById` below. const leftRoots = useMemo( - () => buildFrameworkTree(cfItems, frameworkEdges, rootItemIds), - [cfItems, frameworkEdges, rootItemIds], + () => buildFrameworkTree(frameworkEdges, rootItemIds), + [frameworkEdges, rootItemIds], + ) + + const cfItemsById = useMemo( + () => new Map(cfItems.map((item) => [item.identifier, item])), + [cfItems], ) // Self-alignment target: when the expanded target IS the framework being edited, the right @@ -269,8 +281,13 @@ export default function TreePanelView({ availableFrameworks = [], serverFramewor if (!expandedTargetFramework) return [] const edges = domainFrameworkToEdges(expandedTargetFramework.framework) const roots = domainFrameworkToRootIds(expandedTargetFramework.framework) - return buildFrameworkTree(targetCfItems, edges, roots) - }, [isSelfTarget, leftRoots, expandedTargetFramework, targetCfItems]) + return buildFrameworkTree(edges, roots) + }, [isSelfTarget, leftRoots, expandedTargetFramework]) + + const targetCfItemsById = useMemo( + () => (isSelfTarget ? cfItemsById : new Map(targetCfItems.map((item) => [item.identifier, item]))), + [isSelfTarget, cfItemsById, targetCfItems], + ) // Keep parent maps current so recalculateLines can walk the tree without stale closure issues const leftParentMap = useMemo(() => buildParentMap(leftRoots), [leftRoots]) @@ -539,17 +556,20 @@ export default function TreePanelView({ availableFrameworks = [], serverFramewor // ── Drag handlers ── - const handleSelect = (id: string) => { + // Stable references: FrameworkTreeItem is React.memo'd, so unstable callback + // identities here would force every visible row to re-render on every + // TreePanelView render (defeating the memo). + const handleSelect = useCallback((id: string) => { const deselects = nodes .filter((n) => n.selected && n.id !== id) .map((n) => ({ type: 'select' as const, id: n.id, selected: false })) onNodesChange([...deselects, { type: 'select' as const, id, selected: true }]) - } + }, [nodes, onNodesChange]) - const handleRightDragOver = (id: string) => setDragOverItemId(id) - const handleRightDragLeave = () => setDragOverItemId(null) + const handleRightDragOver = useCallback((id: string) => setDragOverItemId(id), []) + const handleRightDragLeave = useCallback(() => setDragOverItemId(null), []) - const handleRightDrop = (toItemId: string, e: React.DragEvent) => { + const handleRightDrop = useCallback((toItemId: string, e: React.DragEvent) => { const fromItemId = e.dataTransfer.getData('text/plain') if (!fromItemId || !expandedTargetId) return if (fromItemId === toItemId) { @@ -560,8 +580,8 @@ export default function TreePanelView({ availableFrameworks = [], serverFramewor // Resolve canonical URIs from the loaded CFItem data so they survive round-trips, // including the case where the destination framework is on a different server. - const fromCfItem = cfItems.find((i) => i.identifier === fromItemId) - const toCfItem = targetCfItems.find((i) => i.identifier === toItemId) + const fromCfItem = cfItemsById.get(fromItemId) + const toCfItem = targetCfItemsById.get(toItemId) const originUri = fromCfItem?.uri ?? `urn:case:item:${fromItemId}` const destinationUri = toCfItem?.uri ?? `urn:case:item:${toItemId}` @@ -579,7 +599,7 @@ export default function TreePanelView({ availableFrameworks = [], serverFramewor ]) setTargets((prev) => prev.map((t) => (t.id === expandedTargetId ? { ...t, hasUnsavedChanges: true } : t))) setDragOverItemId(null) - } + }, [expandedTargetId, cfItemsById, targetCfItemsById]) const handleAssociationTypeChange = useCallback((id: string, newType: string) => { setPendingAssociations((prev) => { @@ -846,13 +866,14 @@ export default function TreePanelView({ availableFrameworks = [], serverFramewor publisher={cfDocument?.publisher} frameworkNodeId={frameworkNodeId} roots={leftRoots} + cfItemsById={cfItemsById} selectedId={selectedNodeId} expandedIds={leftExpandedIds} onToggleExpand={handleLeftToggleExpand} onSelect={handleSelect} onAddChild={handleAddChild} isDraggable={Boolean(expandedTargetFramework) && isSourcePublished} - onDragStart={() => { /* cursor hint only */ }} + onDragStart={NOOP_DRAG_START} associationCounts={leftAssociationCounts} onBadgeClick={handleLeftBadgeClick} /> @@ -937,6 +958,7 @@ export default function TreePanelView({ availableFrameworks = [], serverFramewor title={fw.cfDocument.title} frameworkNodeId={null} roots={rightRoots} + cfItemsById={targetCfItemsById} selectedId={null} expandedIds={rightExpandedIds} onToggleExpand={handleRightToggleExpand} diff --git a/apps/editor/src/ui/editor/treePanel/buildFrameworkTree.test.ts b/apps/editor/src/ui/editor/treePanel/buildFrameworkTree.test.ts index 25992f4..bb3cc70 100644 --- a/apps/editor/src/ui/editor/treePanel/buildFrameworkTree.test.ts +++ b/apps/editor/src/ui/editor/treePanel/buildFrameworkTree.test.ts @@ -1,54 +1,34 @@ import { describe, it, expect } from 'vitest' import { buildFrameworkTree } from '@/domain/framework/treeDerivation' -import type { CFItem } from '@/domain/case/types' import type { FrameworkEdgeRecord } from '@/domain/framework/treeDerivation' -function makeItem(id: string, overrides: Partial = {}): CFItem { - return { - identifier: id, - uri: `urn:case:item:${id}`, - fullStatement: `Statement for ${id}`, - lastChangeDateTime: '2025-01-01T00:00:00Z', - ...overrides, - } -} - function edge(parentId: string, childId: string, seq?: number): FrameworkEdgeRecord { return { parentId, childId, sequenceNumber: seq } } describe('buildFrameworkTree', () => { - it('returns empty array when cfItems is empty', () => { - expect(buildFrameworkTree([], [], [])).toEqual([]) - }) - it('returns empty array when rootItemIds is empty', () => { - const items = [makeItem('a')] - expect(buildFrameworkTree(items, [], [])).toEqual([]) + expect(buildFrameworkTree([], [])).toEqual([]) }) it('returns root items for a flat list', () => { - const items = [makeItem('a'), makeItem('b'), makeItem('c')] - const tree = buildFrameworkTree(items, [], ['a', 'b', 'c']) + const tree = buildFrameworkTree([], ['a', 'b', 'c']) expect(tree.map((n) => n.id)).toEqual(['a', 'b', 'c']) }) it('root nodes have depth 0', () => { - const items = [makeItem('a'), makeItem('b')] - const tree = buildFrameworkTree(items, [], ['a', 'b']) + const tree = buildFrameworkTree([], ['a', 'b']) for (const node of tree) expect(node.depth).toBe(0) }) it('root nodes have no children when no edges', () => { - const items = [makeItem('a'), makeItem('b')] - const tree = buildFrameworkTree(items, [], ['a', 'b']) + const tree = buildFrameworkTree([], ['a', 'b']) for (const node of tree) expect(node.children).toHaveLength(0) }) it('builds nested children', () => { - const items = [makeItem('a'), makeItem('b'), makeItem('c')] const edges = [edge('a', 'b'), edge('b', 'c')] - const tree = buildFrameworkTree(items, edges, ['a']) + const tree = buildFrameworkTree(edges, ['a']) expect(tree).toHaveLength(1) expect(tree[0].id).toBe('a') expect(tree[0].children).toHaveLength(1) @@ -58,36 +38,40 @@ describe('buildFrameworkTree', () => { expect(tree[0].children[0].children[0].depth).toBe(2) }) - it('includes cfItem data on each node', () => { - const items = [makeItem('a', { humanCodingScheme: 'A.1' })] - const tree = buildFrameworkTree(items, [], ['a']) - expect(tree[0].cfItem.humanCodingScheme).toBe('A.1') - expect(tree[0].cfItem.fullStatement).toBe('Statement for a') - }) - it('respects the order of rootItemIds', () => { - const items = [makeItem('a'), makeItem('b'), makeItem('c')] - const tree = buildFrameworkTree(items, [], ['c', 'a', 'b']) + const tree = buildFrameworkTree([], ['c', 'a', 'b']) expect(tree.map((n) => n.id)).toEqual(['c', 'a', 'b']) }) it('sorts children by sequence number', () => { - const items = [makeItem('parent'), makeItem('x'), makeItem('y'), makeItem('z')] const edges = [edge('parent', 'z', 1), edge('parent', 'x', 2), edge('parent', 'y', 3)] - const tree = buildFrameworkTree(items, edges, ['parent']) + const tree = buildFrameworkTree(edges, ['parent']) expect(tree[0].children.map((n) => n.id)).toEqual(['z', 'x', 'y']) }) it('places children without sequence number after sequenced ones', () => { - const items = [makeItem('parent'), makeItem('a'), makeItem('b')] const edges = [edge('parent', 'b'), edge('parent', 'a', 1)] - const tree = buildFrameworkTree(items, edges, ['parent']) + const tree = buildFrameworkTree(edges, ['parent']) expect(tree[0].children.map((n) => n.id)).toEqual(['a', 'b']) }) - it('skips unknown item IDs silently', () => { - const items = [makeItem('a')] - const tree = buildFrameworkTree(items, [], ['a', 'unknown']) + it('does not include ids not present in rootItemIds/edges, but does not require a separate item list either', () => { + // Shape is derived purely from edges/rootItemIds now — content existence + // is validated separately by callers (via cfItemsById lookups), not here. + const tree = buildFrameworkTree([], ['a']) expect(tree.map((n) => n.id)).toEqual(['a']) }) + + it('guards against cyclic edges instead of recursing forever', () => { + // Malformed/cyclic hierarchical data (e.g. a <-> b) must not cause + // unbounded recursion ("Maximum Call Stack Size Exceeded"). + const edges = [edge('a', 'b'), edge('b', 'a')] + const tree = buildFrameworkTree(edges, ['a']) + expect(tree).toHaveLength(1) + expect(tree[0].id).toBe('a') + expect(tree[0].children).toHaveLength(1) + expect(tree[0].children[0].id).toBe('b') + // b's cyclic edge back to a is dropped, not followed. + expect(tree[0].children[0].children).toHaveLength(0) + }) })