From f7aafb02f9e941f2fe73083afdc67d652c046748 Mon Sep 17 00:00:00 2001 From: Hugo Gu Date: Mon, 25 May 2026 23:09:35 +0800 Subject: [PATCH] fix(web): address three code-review bugs in graph rendering MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Bug 1 (useSigma.ts): forces in the tree physics loop were computed once before the sub-steps loop and reused for every step, causing 2× displacement on slow frames (>64ms, simulationSteps>1). Fix: move forceX/forceY Maps and all force accumulation (layer gravity, edge springs, repulsion, spread) inside the loop so each sub-step integrates from current node positions. Bug 2 (graph-adapter.ts): all three adapters used `graph.hasEdge(src,tgt)` as a dedup guard, which silently drops any second edge between the same node pair. A CALLS relationship between nodes that also have a CONTAINS edge was always lost. Fix: switch from `new Graph()` to `new MultiGraph()` (allows multiple edges per pair) and dedup by `rel.id` instead of by node pair. Bug 3 (graph-adapter.test.ts): the cross-cutting edge styling test never executed its CALLS branch because Bug 2 dropped the CALLS edge before the assertion ran. Fix: assert `sigmaGraph.size === 2` and verify both edges individually after collecting attrs by relationType. Co-authored-by: Claude AI-model: claude-sonnet-4-5 --- gitnexus-web/src/hooks/useSigma.ts | 283 +++++++++++---------- gitnexus-web/src/lib/graph-adapter.test.ts | 46 ++-- gitnexus-web/src/lib/graph-adapter.ts | 24 +- 3 files changed, 192 insertions(+), 161 deletions(-) diff --git a/gitnexus-web/src/hooks/useSigma.ts b/gitnexus-web/src/hooks/useSigma.ts index 15b97012a..6fbbf1a2f 100644 --- a/gitnexus-web/src/hooks/useSigma.ts +++ b/gitnexus-web/src/hooks/useSigma.ts @@ -835,151 +835,156 @@ export const useSigma = (options: UseSigmaOptions = {}): UseSigmaReturn => { treeAccumulatorRef.current -= simulationSteps * TREE_TARGET_FRAME_MS; const dtScale = 0.6; - // --- Accumulate forces --- - const forceX = new Map(); - const forceY = new Map(); - - // 1. Layer gravity: soft pull toward each node's preferred Y within its band. - // Directional nodes (above-only or below-only connections) are pulled to the - // top or bottom of the band; bidirectional / same-layer-only nodes go to - // the center. This leaves band edges free for nodes that actually use them. - graph.forEachNode((nodeId, attrs) => { - const layer = attrs.treeLayer ?? 0; - const centerY = layerCenterY.get(layer) ?? attrs.y; - const bias = nodeYBias.get(nodeId) ?? 0; - const targetY = centerY + bias * TREE_LAYER_BAND_HALF; - forceX.set(nodeId, 0); - forceY.set(nodeId, (targetY - attrs.y) * TREE_LAYER_GRAVITY * dtScale); - }); - - // 2. Edge springs — X and Y handled separately. - // - // Root cause of long horizontal edges: the previous 2D spring projected - // force through (dx/distance, dy/distance). When the Y layer gap - // dominates (|dy|≈200, |dx|≈30) the X component shrinks to ~15% of - // the total spring force, too weak to overcome sibling repulsion. - // - // Fix: compute X spring from |dx| alone. This keeps full strength - // regardless of how far apart two nodes are in Y. - graph.forEachEdge((edge, edgeAttrs, source, target, sourceAttrs, targetAttrs) => { - const dx = targetAttrs.x - sourceAttrs.x; - const rawWeight = TREE_EDGE_WEIGHTS[edgeAttrs.relationType] ?? 0.18; - - // 2a. Pure X spring. - // Hierarchy edges: zero rest length so children want to sit directly - // under their parent (repulsion then spreads siblings out naturally). - // Cross edges: 60 px rest so far-spanning CALLS/IMPORTS edges only - // pull when really stretched, and their weight is capped so they - // don't override the hierarchy structure. - const xRestLength = edgeAttrs.isHierarchyEdge ? 0 : 60; - const xStretch = Math.abs(dx) - xRestLength; - if (xStretch > 0) { - const xWeight = edgeAttrs.isHierarchyEdge ? rawWeight : Math.min(rawWeight, 0.1); - const fxX = Math.sign(dx) * xStretch * xWeight * 0.3 * dtScale; - forceX.set(source, (forceX.get(source) ?? 0) + fxX); - forceX.set(target, (forceX.get(target) ?? 0) - fxX); - } - - // 2b. Weak Y spring — layer gravity handles most vertical placement; - // this just prevents extreme cross-layer stretching. - const dy = targetAttrs.y - sourceAttrs.y; - const distance = Math.sqrt(dx * dx + dy * dy) || 1; - const layerGap = Math.abs((targetAttrs.treeLayer ?? 0) - (sourceAttrs.treeLayer ?? 0)); - const yRestLength = - (edgeAttrs.isHierarchyEdge ? 70 : 95) + - layerGap * (edgeAttrs.isHierarchyEdge ? 28 : 36); - const yStretch = distance - yRestLength; - if (yStretch > 0) { - const fy = (dy / distance) * yStretch * rawWeight * 0.008 * dtScale; - forceY.set(source, (forceY.get(source) ?? 0) + fy); - forceY.set(target, (forceY.get(target) ?? 0) - fy); - } - }); - - // 3. Node repulsion in 2D: all pairs within range (cross-layer included) - // Sort by X for O(n·k) early-exit: once dx > range, all further pairs are too far. - // - // Skipped for large graphs (N > 5 000) — sorting + pair comparisons make each - // frame take hundreds of ms, leaving the canvas apparently frozen. Layer gravity - // and edge springs provide sufficient structure without repulsion. - if (treeUseRepulsion) { - const nodeList = graph.nodes().map((id) => { - const a = graph.getNodeAttributes(id); - return { id, x: a.x, y: a.y, size: a.size ?? 6, layer: a.treeLayer ?? 0 }; - }); - nodeList.sort((a, b) => a.x - b.x); - - for (let i = 0; i < nodeList.length; i++) { - const nodeA = nodeList[i]; - for (let j = i + 1; j < nodeList.length; j++) { - const nodeB = nodeList[j]; - const dx = nodeB.x - nodeA.x; - if (dx > TREE_REPULSION_RANGE) break; // X-sorted: all further pairs are also too far - - const dy = nodeB.y - nodeA.y; - const dist = Math.sqrt(dx * dx + dy * dy) || 1; - if (dist > TREE_REPULSION_RANGE) continue; - - const sameLayer = nodeA.layer === nodeB.layer; - // Same-layer repulsion reduced from 160→100 so the stronger X spring - // (0.30) can now overcome collective repulsion from 3-4 nearby nodes. - // Cross-layer kept low (28) so intermediate-layer nodes don't block - // parent-child X alignment. - const repulsionStrength = sameLayer ? 100 : 28; - const minGap = Math.max(28, (nodeA.size + nodeB.size) * 1.8); - let repulsion = - (1 / (dist + 8) - 1 / (TREE_REPULSION_RANGE + 8)) * repulsionStrength * dtScale; - if (dist < minGap && sameLayer) { - repulsion += (minGap - dist) * 0.1 * dtScale; - } - if (repulsion <= 0) continue; - - const fx = (dx / dist) * repulsion; - const fy = (dy / dist) * repulsion; - - forceX.set(nodeA.id, (forceX.get(nodeA.id) ?? 0) - fx); - forceY.set(nodeA.id, (forceY.get(nodeA.id) ?? 0) - fy); - forceX.set(nodeB.id, (forceX.get(nodeB.id) ?? 0) + fx); - forceY.set(nodeB.id, (forceY.get(nodeB.id) ?? 0) + fy); - } - } - } - - // 4. Spread force: equalize node density within each layer. - // - // For each layer, rank nodes by current X, compute where they would sit - // in a perfectly even distribution, then add a weak force toward that - // ideal position. Nodes that are held by strong hierarchy springs - // (force ≈ 1–2 units) resist and stay clustered; nodes without a - // strong spring anchor (isolated or same-layer-only) drift to fill gaps. - // Net effect: dense centre spreads outward, sparse edges fill in. - // Skipped for large graphs — per-layer sort is O(N log N) per frame. - if (treeUseSpread) { - const spreadByLayer = new Map>(); - graph.forEachNode((nodeId, attrs) => { - const layer = attrs.treeLayer ?? 0; - if (!spreadByLayer.has(layer)) spreadByLayer.set(layer, []); - spreadByLayer.get(layer)!.push({ id: nodeId, x: attrs.x }); - }); - for (const [, layerNodes] of spreadByLayer) { - if (layerNodes.length < 2) continue; - layerNodes.sort((a, b) => a.x - b.x); - const count = layerNodes.length; - const spacing = (TREE_MAX_X * 2) / count; - for (let i = 0; i < count; i++) { - const { id, x } = layerNodes[i]; - const idealX = -TREE_MAX_X + (i + 0.5) * spacing; - forceX.set(id, (forceX.get(id) ?? 0) + (idealX - x) * TREE_SPREAD_STRENGTH * dtScale); - } - } - } - // --- Apply forces: velocity integration with boundary resistance --- + // Forces are recomputed from current node positions each sub-step so that + // slow frames (simulationSteps > 1) integrate correctly and don't double-apply. let totalVelocity = 0; let maxVelocity = 0; let activeNodes = 0; for (let simulationStep = 0; simulationStep < simulationSteps; simulationStep++) { + // --- Accumulate forces (recomputed each sub-step from current positions) --- + const forceX = new Map(); + const forceY = new Map(); + + // 1. Layer gravity: soft pull toward each node's preferred Y within its band. + // Directional nodes (above-only or below-only connections) are pulled to the + // top or bottom of the band; bidirectional / same-layer-only nodes go to + // the center. This leaves band edges free for nodes that actually use them. + graph.forEachNode((nodeId, attrs) => { + const layer = attrs.treeLayer ?? 0; + const centerY = layerCenterY.get(layer) ?? attrs.y; + const bias = nodeYBias.get(nodeId) ?? 0; + const targetY = centerY + bias * TREE_LAYER_BAND_HALF; + forceX.set(nodeId, 0); + forceY.set(nodeId, (targetY - attrs.y) * TREE_LAYER_GRAVITY * dtScale); + }); + + // 2. Edge springs — X and Y handled separately. + // + // Root cause of long horizontal edges: the previous 2D spring projected + // force through (dx/distance, dy/distance). When the Y layer gap + // dominates (|dy|≈200, |dx|≈30) the X component shrinks to ~15% of + // the total spring force, too weak to overcome sibling repulsion. + // + // Fix: compute X spring from |dx| alone. This keeps full strength + // regardless of how far apart two nodes are in Y. + graph.forEachEdge((edge, edgeAttrs, source, target, sourceAttrs, targetAttrs) => { + const dx = targetAttrs.x - sourceAttrs.x; + const rawWeight = TREE_EDGE_WEIGHTS[edgeAttrs.relationType] ?? 0.18; + + // 2a. Pure X spring. + // Hierarchy edges: zero rest length so children want to sit directly + // under their parent (repulsion then spreads siblings out naturally). + // Cross edges: 60 px rest so far-spanning CALLS/IMPORTS edges only + // pull when really stretched, and their weight is capped so they + // don't override the hierarchy structure. + const xRestLength = edgeAttrs.isHierarchyEdge ? 0 : 60; + const xStretch = Math.abs(dx) - xRestLength; + if (xStretch > 0) { + const xWeight = edgeAttrs.isHierarchyEdge ? rawWeight : Math.min(rawWeight, 0.1); + const fxX = Math.sign(dx) * xStretch * xWeight * 0.3 * dtScale; + forceX.set(source, (forceX.get(source) ?? 0) + fxX); + forceX.set(target, (forceX.get(target) ?? 0) - fxX); + } + + // 2b. Weak Y spring — layer gravity handles most vertical placement; + // this just prevents extreme cross-layer stretching. + const dy = targetAttrs.y - sourceAttrs.y; + const distance = Math.sqrt(dx * dx + dy * dy) || 1; + const layerGap = Math.abs((targetAttrs.treeLayer ?? 0) - (sourceAttrs.treeLayer ?? 0)); + const yRestLength = + (edgeAttrs.isHierarchyEdge ? 70 : 95) + + layerGap * (edgeAttrs.isHierarchyEdge ? 28 : 36); + const yStretch = distance - yRestLength; + if (yStretch > 0) { + const fy = (dy / distance) * yStretch * rawWeight * 0.008 * dtScale; + forceY.set(source, (forceY.get(source) ?? 0) + fy); + forceY.set(target, (forceY.get(target) ?? 0) - fy); + } + }); + + // 3. Node repulsion in 2D: all pairs within range (cross-layer included) + // Sort by X for O(n·k) early-exit: once dx > range, all further pairs are too far. + // + // Skipped for large graphs (N > 5 000) — sorting + pair comparisons make each + // frame take hundreds of ms, leaving the canvas apparently frozen. Layer gravity + // and edge springs provide sufficient structure without repulsion. + if (treeUseRepulsion) { + const nodeList = graph.nodes().map((id) => { + const a = graph.getNodeAttributes(id); + return { id, x: a.x, y: a.y, size: a.size ?? 6, layer: a.treeLayer ?? 0 }; + }); + nodeList.sort((a, b) => a.x - b.x); + + for (let i = 0; i < nodeList.length; i++) { + const nodeA = nodeList[i]; + for (let j = i + 1; j < nodeList.length; j++) { + const nodeB = nodeList[j]; + const dx = nodeB.x - nodeA.x; + if (dx > TREE_REPULSION_RANGE) break; // X-sorted: all further pairs are also too far + + const dy = nodeB.y - nodeA.y; + const dist = Math.sqrt(dx * dx + dy * dy) || 1; + if (dist > TREE_REPULSION_RANGE) continue; + + const sameLayer = nodeA.layer === nodeB.layer; + // Same-layer repulsion reduced from 160→100 so the stronger X spring + // (0.30) can now overcome collective repulsion from 3-4 nearby nodes. + // Cross-layer kept low (28) so intermediate-layer nodes don't block + // parent-child X alignment. + const repulsionStrength = sameLayer ? 100 : 28; + const minGap = Math.max(28, (nodeA.size + nodeB.size) * 1.8); + let repulsion = + (1 / (dist + 8) - 1 / (TREE_REPULSION_RANGE + 8)) * repulsionStrength * dtScale; + if (dist < minGap && sameLayer) { + repulsion += (minGap - dist) * 0.1 * dtScale; + } + if (repulsion <= 0) continue; + + const fx = (dx / dist) * repulsion; + const fy = (dy / dist) * repulsion; + + forceX.set(nodeA.id, (forceX.get(nodeA.id) ?? 0) - fx); + forceY.set(nodeA.id, (forceY.get(nodeA.id) ?? 0) - fy); + forceX.set(nodeB.id, (forceX.get(nodeB.id) ?? 0) + fx); + forceY.set(nodeB.id, (forceY.get(nodeB.id) ?? 0) + fy); + } + } + } + + // 4. Spread force: equalize node density within each layer. + // + // For each layer, rank nodes by current X, compute where they would sit + // in a perfectly even distribution, then add a weak force toward that + // ideal position. Nodes that are held by strong hierarchy springs + // (force ≈ 1–2 units) resist and stay clustered; nodes without a + // strong spring anchor (isolated or same-layer-only) drift to fill gaps. + // Net effect: dense centre spreads outward, sparse edges fill in. + // Skipped for large graphs — per-layer sort is O(N log N) per frame. + if (treeUseSpread) { + const spreadByLayer = new Map>(); + graph.forEachNode((nodeId, attrs) => { + const layer = attrs.treeLayer ?? 0; + if (!spreadByLayer.has(layer)) spreadByLayer.set(layer, []); + spreadByLayer.get(layer)!.push({ id: nodeId, x: attrs.x }); + }); + for (const [, layerNodes] of spreadByLayer) { + if (layerNodes.length < 2) continue; + layerNodes.sort((a, b) => a.x - b.x); + const count = layerNodes.length; + const spacing = (TREE_MAX_X * 2) / count; + for (let i = 0; i < count; i++) { + const { id, x } = layerNodes[i]; + const idealX = -TREE_MAX_X + (i + 0.5) * spacing; + forceX.set( + id, + (forceX.get(id) ?? 0) + (idealX - x) * TREE_SPREAD_STRENGTH * dtScale, + ); + } + } + } + totalVelocity = 0; maxVelocity = 0; activeNodes = 0; diff --git a/gitnexus-web/src/lib/graph-adapter.test.ts b/gitnexus-web/src/lib/graph-adapter.test.ts index 52e2cd93d..a947cca73 100644 --- a/gitnexus-web/src/lib/graph-adapter.test.ts +++ b/gitnexus-web/src/lib/graph-adapter.test.ts @@ -63,16 +63,23 @@ describe('knowledgeGraphToTreeGraphology', () => { const sigmaGraph = knowledgeGraphToTreeGraphology(graph); - // Find edges and check their attributes - sigmaGraph.forEachEdge((edge, attrs) => { - if (attrs.relationType === 'CONTAINS') { - expect(attrs.isHierarchyEdge).toBe(true); - expect(attrs.color).toBe(EDGE_INFO.CONTAINS.color); - } else if (attrs.relationType === 'CALLS') { - expect(attrs.isHierarchyEdge).toBe(false); - expect(attrs.color).toBe(EDGE_INFO.CALLS.color); - } + // MultiGraph allows multiple edges per pair — both CONTAINS and CALLS must survive. + expect(sigmaGraph.size).toBe(2); + + const attrsByType = new Map(); + sigmaGraph.forEachEdge((_edge, attrs) => { + attrsByType.set(attrs.relationType, attrs); }); + + const containsAttrs = attrsByType.get('CONTAINS'); + expect(containsAttrs).toBeDefined(); + expect(containsAttrs!.isHierarchyEdge).toBe(true); + expect(containsAttrs!.color).toBe(EDGE_INFO.CONTAINS.color); + + const callsAttrs = attrsByType.get('CALLS'); + expect(callsAttrs).toBeDefined(); + expect(callsAttrs!.isHierarchyEdge).toBe(false); + expect(callsAttrs!.color).toBe(EDGE_INFO.CALLS.color); }); it('should treat imports as cross-cutting edges in tree view', () => { @@ -161,16 +168,25 @@ describe('knowledgeGraphToCirclesGraphology', () => { ], }; - // Two relationships between the same pair — graph-adapter deduplicates via - // hasEdge check, so only the first inserted (CONTAINS, hierarchy) is kept. + // MultiGraph allows multiple edges per pair — both CONTAINS and CALLS must survive. const sigmaGraph = knowledgeGraphToCirclesGraphology(graph); + expect(sigmaGraph.size).toBe(2); + + const attrsByType = new Map(); sigmaGraph.forEachEdge((_, attrs) => { - if (attrs.relationType === 'CONTAINS') { - expect(attrs.isHierarchyEdge).toBe(true); - expect(attrs.color).toBe(EDGE_INFO.CONTAINS.color); - } + attrsByType.set(attrs.relationType, attrs); }); + + const containsAttrs = attrsByType.get('CONTAINS'); + expect(containsAttrs).toBeDefined(); + expect(containsAttrs!.isHierarchyEdge).toBe(true); + expect(containsAttrs!.color).toBe(EDGE_INFO.CONTAINS.color); + + const callsAttrs = attrsByType.get('CALLS'); + expect(callsAttrs).toBeDefined(); + expect(callsAttrs!.isHierarchyEdge).toBe(false); + expect(callsAttrs!.color).toBe(EDGE_INFO.CALLS.color); }); it('should treat CALLS as a cross-cutting edge in circles view', () => { diff --git a/gitnexus-web/src/lib/graph-adapter.ts b/gitnexus-web/src/lib/graph-adapter.ts index a896b3624..8f8c583ca 100644 --- a/gitnexus-web/src/lib/graph-adapter.ts +++ b/gitnexus-web/src/lib/graph-adapter.ts @@ -1,4 +1,4 @@ -import Graph from 'graphology'; +import Graph, { MultiGraph } from 'graphology'; import type { NodeLabel } from 'gitnexus-shared'; import type { KnowledgeGraph } from '../core/graph/types'; import { EDGE_INFO, NODE_COLORS, NODE_SIZES, getCommunityColor } from './constants'; @@ -95,7 +95,7 @@ export const knowledgeGraphToGraphology = ( knowledgeGraph: KnowledgeGraph, communityMemberships?: Map, ): Graph => { - const graph = new Graph(); + const graph = new MultiGraph(); const nodeCount = knowledgeGraph.nodes.length; // Build parent-child map from hierarchy relationships @@ -314,9 +314,13 @@ export const knowledgeGraphToGraphology = ( // and cross-edges (CALLS, IMPORTS, EXTENDS) are drawn on top. const BACKGROUND_EDGE_TYPES = new Set(['CONTAINS', 'DEFINES', 'HAS_METHOD', 'HAS_PROPERTY']); + // Dedup by relationship ID, not by node-pair — a node pair can have both a + // CONTAINS edge and a CALLS edge (MultiGraph allows multiple edges per pair). + const addedRelIds = new Set(); const addEdge = (rel: (typeof knowledgeGraph.relationships)[number]) => { if (!graph.hasNode(rel.sourceId) || !graph.hasNode(rel.targetId)) return; - if (graph.hasEdge(rel.sourceId, rel.targetId)) return; + if (addedRelIds.has(rel.id)) return; + addedRelIds.add(rel.id); const style = EDGE_STYLES[rel.type] || { color: '#4a4a5a', sizeMultiplier: 0.5 }; const curvature = 0.12 + Math.random() * 0.08; graph.addEdge(rel.sourceId, rel.targetId, { @@ -343,7 +347,7 @@ export const knowledgeGraphToGraphology = ( export const knowledgeGraphToTreeGraphology = ( knowledgeGraph: KnowledgeGraph, ): Graph => { - const graph = new Graph(); + const graph = new MultiGraph(); const nodeCount = knowledgeGraph.nodes.length; const positions = calculateTreeLayout(knowledgeGraph); @@ -392,9 +396,12 @@ export const knowledgeGraphToTreeGraphology = ( }; // Two-pass insertion: hierarchy edges first (rendered behind), cross-edges on top. + // Dedup by relationship ID so CONTAINS + CALLS between the same pair both survive. + const addedTreeRelIds = new Set(); const addTreeEdge = (rel: (typeof knowledgeGraph.relationships)[number]) => { if (!graph.hasNode(rel.sourceId) || !graph.hasNode(rel.targetId)) return; - if (graph.hasEdge(rel.sourceId, rel.targetId)) return; + if (addedTreeRelIds.has(rel.id)) return; + addedTreeRelIds.add(rel.id); const isHierarchy = HIERARCHY_EDGE_STYLES[rel.type] !== undefined; const style = isHierarchy ? HIERARCHY_EDGE_STYLES[rel.type] @@ -422,7 +429,7 @@ export const knowledgeGraphToTreeGraphology = ( export const knowledgeGraphToCirclesGraphology = ( knowledgeGraph: KnowledgeGraph, ): Graph => { - const graph = new Graph(); + const graph = new MultiGraph(); const nodeCount = knowledgeGraph.nodes.length; const positions = calculateCirclesLayout(knowledgeGraph); @@ -471,9 +478,12 @@ export const knowledgeGraphToCirclesGraphology = ( }; // Two-pass insertion: hierarchy edges first (rendered behind), cross-edges on top. + // Dedup by relationship ID so CONTAINS + CALLS between the same pair both survive. + const addedCirclesRelIds = new Set(); const addCirclesEdge = (rel: (typeof knowledgeGraph.relationships)[number]) => { if (!graph.hasNode(rel.sourceId) || !graph.hasNode(rel.targetId)) return; - if (graph.hasEdge(rel.sourceId, rel.targetId)) return; + if (addedCirclesRelIds.has(rel.id)) return; + addedCirclesRelIds.add(rel.id); const isHierarchy = HIERARCHY_EDGE_STYLES[rel.type] !== undefined; const style = isHierarchy ? HIERARCHY_EDGE_STYLES[rel.type]