From 7e41ba7a68e3be0567b7d21c7519f0bd302d00a3 Mon Sep 17 00:00:00 2001 From: Lum1104 Date: Sun, 12 Apr 2026 11:21:24 +0800 Subject: [PATCH] =?UTF-8?q?fix:=20address=20Codex=20review=20=E2=80=94=20s?= =?UTF-8?q?table=20layout,=20scoped=20infra=20filter,=20basename=20dedup?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit P1: Separate force layout computation from visual state updates so clicking/searching/touring doesn't re-randomize node positions. Layout only recomputes when the graph data or filters change. P1: Restrict infrastructure-file skipping (index.md, log.md, etc.) to the wiki root level only. Nested files like concepts/index.md are now correctly treated as content articles. P2: Track ambiguous bare basenames in the wikilink resolution map. Duplicate basenames (e.g., a/foo.md and b/foo.md) are removed from the flat lookup so [[foo]] doesn't silently resolve to the wrong page. Also fixed: edge IDs now use stable source-target-type keys instead of array indices for proper React reconciliation. Co-Authored-By: Claude Opus 4.6 (1M context) --- .../src/components/KnowledgeGraphView.tsx | 203 +++++++++--------- .../parse-knowledge-base.py | 33 ++- 2 files changed, 132 insertions(+), 104 deletions(-) diff --git a/understand-anything-plugin/packages/dashboard/src/components/KnowledgeGraphView.tsx b/understand-anything-plugin/packages/dashboard/src/components/KnowledgeGraphView.tsx index 848d9a6..1d05f48 100644 --- a/understand-anything-plugin/packages/dashboard/src/components/KnowledgeGraphView.tsx +++ b/understand-anything-plugin/packages/dashboard/src/components/KnowledgeGraphView.tsx @@ -37,7 +37,6 @@ const EDGE_STYLES: Record = { function getNodeDimensions( edgeCount: number, ): { width: number; height: number } { - // Scale width/height by degree (connections) const scale = Math.min(1.5, Math.max(0.85, 0.85 + edgeCount * 0.03)); return { width: Math.round(NODE_WIDTH * scale), @@ -45,22 +44,19 @@ function getNodeDimensions( }; } -function buildKnowledgeGraph( +/** + * Compute the stable layout (positions) from graph topology. + * This only re-runs when the graph data or filters change, NOT on selection/search. + */ +function computeLayout( graph: KnowledgeGraph, - selectedNodeId: string | null, - focusNodeId: string | null, - searchResults: Map, - tourHighlightedNodeIds: Set, - onNodeClick: (nodeId: string) => void, -): { nodes: Node[]; edges: Edge[] } { - // Count edges per node for degree-proportional sizing +): { positionMap: Map; edgeCounts: Map; communityMap: Map } { const edgeCounts = new Map(); for (const edge of graph.edges) { edgeCounts.set(edge.source, (edgeCounts.get(edge.source) ?? 0) + 1); edgeCounts.set(edge.target, (edgeCounts.get(edge.target) ?? 0) + 1); } - // Build community map from layers const communityMap = new Map(); graph.layers.forEach((layer, i) => { for (const nodeId of layer.nodeIds) { @@ -68,83 +64,33 @@ function buildKnowledgeGraph( } }); - // Determine neighbor IDs for focus/selection fading - const neighborIds = new Set(); - if (focusNodeId || selectedNodeId) { - const focusId = focusNodeId ?? selectedNodeId; - for (const edge of graph.edges) { - if (edge.source === focusId) neighborIds.add(edge.target); - if (edge.target === focusId) neighborIds.add(edge.source); - } - } - - // Build node dimensions map const dims = new Map(); for (const node of graph.nodes) { - const d = getNodeDimensions(edgeCounts.get(node.id) ?? 0); - dims.set(node.id, d); + dims.set(node.id, getNodeDimensions(edgeCounts.get(node.id) ?? 0)); } - // Build xyflow nodes - const rfNodes: Node[] = graph.nodes.map((node) => { - const isSelected = node.id === selectedNodeId; - const isFocused = node.id === focusNodeId; - const isNeighbor = neighborIds.has(node.id); - const isSelectionFaded = - (focusNodeId || selectedNodeId) && - !isSelected && - !isFocused && - !isNeighbor; - const searchScore = searchResults.get(node.id); - const isHighlighted = searchScore !== undefined; - const isTourHighlighted = tourHighlightedNodeIds.has(node.id); + // Build temporary nodes/edges for layout computation only + const tmpNodes: Node[] = graph.nodes.map((node) => ({ + id: node.id, + type: "custom" as const, + position: { x: 0, y: 0 }, + data: {}, + })); - const data: CustomNodeData = { - label: node.name, - nodeType: node.type, - summary: node.summary, - complexity: node.complexity, - isHighlighted, - searchScore, - isSelected, - isTourHighlighted, - isDiffChanged: false, - isDiffAffected: false, - isDiffFaded: false, - isNeighbor, - isSelectionFaded: !!isSelectionFaded, - onNodeClick, - incomingCount: edgeCounts.get(node.id) ?? 0, - tags: node.tags, - }; + const tmpEdges: Edge[] = graph.edges.map((e, i) => ({ + id: `ke-${i}`, + source: e.source, + target: e.target, + })); - return { - id: node.id, - type: "custom" as const, - position: { x: 0, y: 0 }, - data, - }; - }); + const { nodes: layoutedNodes } = applyForceLayout(tmpNodes, tmpEdges, dims, communityMap); - // Build xyflow edges - const rfEdges: Edge[] = graph.edges.map((e, i) => { - const style = EDGE_STYLES[e.type] ?? EDGE_STYLES.related; - return { - id: `ke-${i}-${e.source}-${e.target}`, - source: e.source, - target: e.target, - style, - animated: e.type === "contradicts", - label: e.type !== "related" && e.type !== "categorized_under" ? e.type.replace(/_/g, " ") : undefined, - labelStyle: { fill: "var(--color-text-muted)", fontSize: 9, opacity: 0.7 }, - labelBgStyle: { fill: "var(--color-surface)", fillOpacity: 0.9 }, - labelBgPadding: [4, 2] as [number, number], - labelBgBorderRadius: 3, - }; - }); + const positionMap = new Map(); + for (const n of layoutedNodes) { + positionMap.set(n.id, n.position); + } - // Apply force layout with community clustering - return applyForceLayout(rfNodes, rfEdges, dims, communityMap); + return { positionMap, edgeCounts, communityMap }; } function KnowledgeGraphViewInner() { @@ -171,10 +117,10 @@ function KnowledgeGraphViewInner() { [tourHighlightedNodeIds], ); - const { nodes, edges } = useMemo(() => { - if (!graph) return { nodes: [], edges: [] }; + // Filter graph — only recompute when graph data or filters change + const filteredGraph = useMemo((): KnowledgeGraph | null => { + if (!graph) return null; - // Filter graph by active node type filters const filteredNodes = graph.nodes.filter((n) => { if (["article", "entity", "topic", "claim", "source"].includes(n.type)) { return nodeTypeFilters.knowledge !== false; @@ -187,21 +133,86 @@ function KnowledgeGraphViewInner() { (e) => filteredNodeIds.has(e.source) && filteredNodeIds.has(e.target), ); - const filteredGraph: KnowledgeGraph = { - ...graph, - nodes: filteredNodes, - edges: filteredEdges, - }; + return { ...graph, nodes: filteredNodes, edges: filteredEdges }; + }, [graph, nodeTypeFilters]); - return buildKnowledgeGraph( - filteredGraph, - selectedNodeId, - focusNodeId, - searchResults, - tourSet, - onNodeClick, - ); - }, [graph, selectedNodeId, focusNodeId, searchResults, tourSet, onNodeClick, nodeTypeFilters]); + // Compute layout ONCE per graph/filter change — stable positions + const { positionMap, edgeCounts } = useMemo(() => { + if (!filteredGraph) return { positionMap: new Map(), edgeCounts: new Map() }; + return computeLayout(filteredGraph); + }, [filteredGraph]); + + // Build visual nodes/edges — recomputes on selection/search/tour WITHOUT re-layout + const { nodes, edges } = useMemo(() => { + if (!filteredGraph) return { nodes: [], edges: [] }; + + const neighborIds = new Set(); + if (focusNodeId || selectedNodeId) { + const focusId = focusNodeId ?? selectedNodeId; + for (const edge of filteredGraph.edges) { + if (edge.source === focusId) neighborIds.add(edge.target); + if (edge.target === focusId) neighborIds.add(edge.source); + } + } + + const rfNodes: Node[] = filteredGraph.nodes.map((node) => { + const isSelected = node.id === selectedNodeId; + const isFocused = node.id === focusNodeId; + const isNeighbor = neighborIds.has(node.id); + const isSelectionFaded = + (focusNodeId || selectedNodeId) && + !isSelected && + !isFocused && + !isNeighbor; + const searchScore = searchResults.get(node.id); + const isHighlighted = searchScore !== undefined; + const isTourHighlighted = tourSet.has(node.id); + + const data: CustomNodeData = { + label: node.name, + nodeType: node.type, + summary: node.summary, + complexity: node.complexity, + isHighlighted, + searchScore, + isSelected, + isTourHighlighted, + isDiffChanged: false, + isDiffAffected: false, + isDiffFaded: false, + isNeighbor, + isSelectionFaded: !!isSelectionFaded, + onNodeClick, + incomingCount: edgeCounts.get(node.id) ?? 0, + tags: node.tags, + }; + + return { + id: node.id, + type: "custom" as const, + position: positionMap.get(node.id) ?? { x: 0, y: 0 }, + data, + }; + }); + + const rfEdges: Edge[] = filteredGraph.edges.map((e) => { + const style = EDGE_STYLES[e.type] ?? EDGE_STYLES.related; + return { + id: `ke-${e.source}-${e.target}-${e.type}`, + source: e.source, + target: e.target, + style, + animated: e.type === "contradicts", + label: e.type !== "related" && e.type !== "categorized_under" ? e.type.replace(/_/g, " ") : undefined, + labelStyle: { fill: "var(--color-text-muted)", fontSize: 9, opacity: 0.7 }, + labelBgStyle: { fill: "var(--color-surface)", fillOpacity: 0.9 }, + labelBgPadding: [4, 2] as [number, number], + labelBgBorderRadius: 3, + }; + }); + + return { nodes: rfNodes, edges: rfEdges }; + }, [filteredGraph, selectedNodeId, focusNodeId, searchResults, tourSet, onNodeClick, positionMap, edgeCounts]); if (!graph) { return ( diff --git a/understand-anything-plugin/skills/understand-knowledge/parse-knowledge-base.py b/understand-anything-plugin/skills/understand-knowledge/parse-knowledge-base.py index 10c6b2e..45d95e4 100644 --- a/understand-anything-plugin/skills/understand-knowledge/parse-knowledge-base.py +++ b/understand-anything-plugin/skills/understand-knowledge/parse-knowledge-base.py @@ -221,15 +221,31 @@ def parse_log(log_path: Path) -> list[dict]: # --------------------------------------------------------------------------- def build_name_to_stem_map(wiki_root: Path) -> dict[str, str]: - """Build a case-insensitive map from filename stem to relative stem path.""" + """Build a case-insensitive map from filename stem to relative stem path. + + Full relative paths always map uniquely. Bare basenames map only when + unambiguous — duplicate basenames are removed so they don't silently + resolve to the wrong page. + """ name_map: dict[str, str] = {} + # Track which bare basenames appear more than once + basename_counts: dict[str, int] = {} for md_file in wiki_root.rglob("*.md"): rel = md_file.relative_to(wiki_root) stem = str(rel.with_suffix("")) # e.g., "decisions/decision-foo" basename = md_file.stem # e.g., "decision-foo" - # Map both full relative path and bare filename (for flat wikilink resolution) + # Full relative path always maps uniquely name_map[stem.lower()] = stem - name_map[basename.lower()] = stem + # Track basename for ambiguity detection + key = basename.lower() + basename_counts[key] = basename_counts.get(key, 0) + 1 + name_map[key] = stem + + # Remove ambiguous basename entries (appear more than once) + for key, count in basename_counts.items(): + if count > 1 and key in name_map: + del name_map[key] + return name_map @@ -292,13 +308,14 @@ def parse_wiki(root: Path) -> dict: category_lookup[article_target.lower()] = cat["name"] # --- Pre-compute article IDs (for edge resolution validation) --- - # Must use the same filter logic as the main loop (skip if EITHER matches INFRA_FILES) + # Only skip infra files at the wiki root level, not in subdirectories + # (e.g., wiki/index.md is infra, but wiki/concepts/index.md is content) article_ids: set[str] = set() for md_file in sorted(wiki_root.rglob("*.md")): rel = md_file.relative_to(wiki_root) stem = str(rel.with_suffix("")) - basename = md_file.stem - if basename.lower() in INFRA_FILES or rel.name.lower() in INFRA_FILES: + # Only filter infra files at root level (no parent directory) + if rel.parent == Path(".") and rel.name.lower() in INFRA_FILES: continue article_ids.add(f"article:{stem}") @@ -313,8 +330,8 @@ def parse_wiki(root: Path) -> dict: stem = str(rel.with_suffix("")) basename = md_file.stem - # Skip infrastructure files - if basename.lower() in INFRA_FILES or rel.name.lower() in INFRA_FILES: + # Skip infrastructure files only at wiki root level + if rel.parent == Path(".") and rel.name.lower() in INFRA_FILES: continue text = md_file.read_text(encoding="utf-8", errors="replace")