From 3fbe3cfd2d3bf340b2698a112e4aa6f9159715d5 Mon Sep 17 00:00:00 2001 From: Sameer6305 Date: Tue, 18 Aug 2026 20:34:54 +0530 Subject: [PATCH] fix(explorer): address edge label review findings --- .../workspaces/GraphWorkspace/GraphCanvas.tsx | 12 +++++- .../GraphWorkspace/graphSceneState.ts | 28 +++++++++--- .../plugins/explorationEffectsPlugin.tsx | 26 ++++++----- .../tests/graphSceneState.display.test.ts | 43 +++++++++++++++++++ 4 files changed, 90 insertions(+), 19 deletions(-) diff --git a/explorer/src/workspaces/GraphWorkspace/GraphCanvas.tsx b/explorer/src/workspaces/GraphWorkspace/GraphCanvas.tsx index 698a9a1e..bb9abe1f 100644 --- a/explorer/src/workspaces/GraphWorkspace/GraphCanvas.tsx +++ b/explorer/src/workspaces/GraphWorkspace/GraphCanvas.tsx @@ -1312,6 +1312,9 @@ export const GraphCanvas = forwardRef( const onEdgeClickRef = useRef(onEdgeClick); const onSceneRuntimeChangeRef = useRef(onSceneRuntimeChange); const onCameraStateChangeRef = useRef(onCameraStateChange); + // #1009: tracked as a ref so the Sigma creation effect always reads the + // current value without needing effectsState in its dependency array. + const effectsStateRef = useRef(effectsState); const [hoveredNodeId, setHoveredNodeId] = useState(null); const [zoomTier, setZoomTier] = useState("overview"); const [analyticsSnapshot, setAnalyticsSnapshot] = useState(null); @@ -1340,6 +1343,7 @@ export const GraphCanvas = forwardRef( onEdgeClickRef.current = onEdgeClick; onSceneRuntimeChangeRef.current = onSceneRuntimeChange; onCameraStateChangeRef.current = onCameraStateChange; + effectsStateRef.current = effectsState; const behaviors = useMemo( () => [ @@ -1852,7 +1856,13 @@ export const GraphCanvas = forwardRef( return; } - const sigma = new Sigma(displayGraphRef.current, containerRef.current, SIGMA_SETTINGS); + const sigma = new Sigma(displayGraphRef.current, containerRef.current, { + ...SIGMA_SETTINGS, + // #1009: initialize with the current toggle value rather than the + // static default so that a user who disabled Edge Labels before + // graph/Sigma initialization sees the correct state after mount. + renderEdgeLabels: effectsStateRef.current.edgeLabelsEnabled, + }); sigmaRef.current = sigma; appliedGraphVersionRef.current = graphVersionRef.current; diff --git a/explorer/src/workspaces/GraphWorkspace/graphSceneState.ts b/explorer/src/workspaces/GraphWorkspace/graphSceneState.ts index eb468bf3..5fa0d766 100644 --- a/explorer/src/workspaces/GraphWorkspace/graphSceneState.ts +++ b/explorer/src/workspaces/GraphWorkspace/graphSceneState.ts @@ -2099,6 +2099,15 @@ function createCollapsedNeighborhoodGraph( return collapsedGraph; } +// Normalize an edge relationship type: empty string, null, and undefined all +// fall back to the project-wide default used consistently across every +// aggregation path. Keep this local — it exists only to guarantee that the +// three code paths (single-entry, multi-entry, community-grouped) produce the +// same semantics and do not diverge again. +function normalizeEdgeType(value: string | null | undefined): string { + return value || "related_to"; +} + function aggregateDisplayGraph(graphRef: GraphRef): Graph { const aggregated = new Graph({ type: "directed", @@ -2124,10 +2133,13 @@ function aggregateDisplayGraph(graphRef: GraphRef): Graph collectRawEdgeIds(attrs, edgeId)); const typeCounts = new Map(); entries.forEach(({ attrs }) => { - const edgeType = String(attrs.edgeType || "related_to"); + const edgeType = normalizeEdgeType(attrs.edgeType); typeCounts.set(edgeType, (typeCounts.get(edgeType) ?? 0) + 1); }); - const dominantEdgeType = [...typeCounts.entries()].sort((left, right) => right[1] - left[1])[0]?.[0] ?? representative.attrs.edgeType ?? "related_to"; + const dominantEdgeType = [...typeCounts.entries()].sort((left, right) => right[1] - left[1])[0]?.[0] + ?? normalizeEdgeType(representative.attrs.edgeType); const reverseKey = `${targetId}→${sourceId}`; const isBidirectionalBundle = groupedEdges.has(reverseKey); const syntheticEdgeId = `${AGGREGATED_EDGE_PREFIX}${sourceId}::${targetId}`; @@ -2167,10 +2180,10 @@ function aggregateDisplayGraph(graphRef: GraphRef): Graph right[1] - left[1])[0]?.[0] ?? "related_to"; + const dominantEdgeType = [...bundle.typeCounts.entries()].sort((left, right) => right[1] - left[1])[0]?.[0] + ?? "related_to"; const reverseKey = `${bundle.targetId}→${bundle.sourceId}`; const syntheticEdgeId = `${AGGREGATED_EDGE_PREFIX}${key}`; const aggregateCount = bundle.rawEdgeIds.length; diff --git a/explorer/src/workspaces/GraphWorkspace/plugins/explorationEffectsPlugin.tsx b/explorer/src/workspaces/GraphWorkspace/plugins/explorationEffectsPlugin.tsx index ba779699..4bae05d8 100644 --- a/explorer/src/workspaces/GraphWorkspace/plugins/explorationEffectsPlugin.tsx +++ b/explorer/src/workspaces/GraphWorkspace/plugins/explorationEffectsPlugin.tsx @@ -1,6 +1,7 @@ import type { CSSProperties } from "react"; import type { + GraphDiagnosticsSnapshot, GraphEffectAvailability, GraphEffectToggle, } from "../types"; @@ -42,6 +43,17 @@ const EFFECT_ROWS: EffectRowConfig[] = [ }, ]; +// Maps the effect toggle keys rendered by this plugin to their corresponding +// availability keys in GraphDiagnosticsSnapshot["effectAvailability"]. Kept +// local because this plugin only renders a subset of all effects. +const EFFECT_AVAILABILITY_KEYS: Partial> = { + pathPulseEnabled: "pathPulse", + pathFlowEnabled: "pathFlow", + lensEnabled: "lens", + edgeLabelsEnabled: "edgeLabels", + legendEnabled: "legend", +}; + function renderAvailabilityText(availability: GraphEffectAvailability) { if (availability.available) { if (typeof availability.visibleSegments === "number" && typeof availability.segmentCap === "number") { @@ -144,17 +156,9 @@ export const explorationEffectsPlugin: GraphPlugin = { description={row.description} checked={effectsState[row.key]} availability={ - availability?.[ - row.key === "pathPulseEnabled" - ? "pathPulse" - : row.key === "pathFlowEnabled" - ? "pathFlow" - : row.key === "lensEnabled" - ? "lens" - : row.key === "edgeLabelsEnabled" - ? "edgeLabels" - : "legend" - ] ?? { + (EFFECT_AVAILABILITY_KEYS[row.key] !== undefined + ? availability?.[EFFECT_AVAILABILITY_KEYS[row.key]!] + : undefined) ?? { enabled: effectsState[row.key], available: false, reason: "Waiting for graph runtime", diff --git a/explorer/tests/graphSceneState.display.test.ts b/explorer/tests/graphSceneState.display.test.ts index d7e226cd..6621ba86 100644 --- a/explorer/tests/graphSceneState.display.test.ts +++ b/explorer/tests/graphSceneState.display.test.ts @@ -1213,3 +1213,46 @@ test("resolveEdgeElementStyle hidden class produces hidden:true for suppressed e ); assert.equal(style.hidden, true); }); + +// ── #1009 maintainer-blocking regression: single-edge empty edgeType ───────── + +test("resolveDisplayGraph single-edge normalizes empty-string edgeType to related_to", () => { + addNode("a"); + addNode("b"); + // One edge only — exercises the entries.length === 1 path in aggregateDisplayGraph. + batchMergeEdges([{ + id: "e-single-empty", + source: "a", + target: "b", + attributes: { edgeType: "", weight: 1, properties: {} }, + }]); + + const { graph: displayGraph } = resolveDisplayGraph("", [], [], "full", { aggregationEnabled: true }); + assert.equal(displayGraph.size, 1); + + const edgeId = displayGraph.edges()[0]; + const attrs = displayGraph.getEdgeAttributes(edgeId) as { edgeType?: string; dominantEdgeType?: string }; + assert.equal(attrs.edgeType, "related_to", + "single-edge path must normalize empty edgeType to the canonical fallback"); + assert.equal(attrs.dominantEdgeType, "related_to", + "single-edge dominantEdgeType must also be normalized"); +}); + +test("resolveDisplayGraph single-edge preserves a valid non-empty edgeType unchanged", () => { + addNode("a"); + addNode("b"); + batchMergeEdges([{ + id: "e-single-valid", + source: "a", + target: "b", + attributes: { edgeType: "works_for", weight: 1, properties: {} }, + }]); + + const { graph: displayGraph } = resolveDisplayGraph("", [], [], "full", { aggregationEnabled: true }); + assert.equal(displayGraph.size, 1); + + const edgeId = displayGraph.edges()[0]; + const attrs = displayGraph.getEdgeAttributes(edgeId) as { edgeType?: string }; + assert.equal(attrs.edgeType, "works_for", + "single-edge path must not alter a valid relationship type"); +});