From 988533a550bf945a6dcd80c32c092384bf0f5669 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?d=20=F0=9F=94=B9?= <258577966+voidborne-d@users.noreply.github.com> Date: Mon, 4 May 2026 17:16:42 +0800 Subject: [PATCH] fix(dashboard): preserve any-layer-wins membership for filterNodes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reviewer @Lum1104 (PR #112) caught a silent semantic regression: the new `filterNodes` reads layer membership through `nodeIdToLayerId.get(node.id)`, which is first-wins. The pre-#112 path was any-layer-wins — `layers.some(layer => filters.layerIds.has(layer.id) && layer.nodeIds.includes(node.id))`. For a node X listed in both L1 and L2 with only L2 selected, the old code kept X; the new code dropped it. The schema permits multi-layer membership, so this was a behavior change, not a bug fix. Fix: keep two distinct indexes in the store. Both are rebuilt once on `setGraph`, so the O(1)-per-node performance win from #112 is preserved. - `nodeIdToLayerId: Map` — first-matching-layer wins. Drives navigation (drillIntoLayer / tour step → layer / sidebar history) where one canonical layer is the right answer. Unchanged. - `nodeIdToLayerIds: Map>` — every layer the node belongs to. Drives `filterNodes` membership checks. Restores any-layer-wins exactly. `filterNodes` now iterates the (small) layer-id set per node looking for intersection with `filters.layerIds`. ExportMenu reads `nodeIdToLayerIds` from the store. Verified locally: - Added `filters.test.ts` regression: node in (L1, L2) with only L2 selected must pass. Failed against the first-wins implementation; passes now. - `pnpm --filter @understand-anything/dashboard test` — 42 / 42 pass (was 41; +1 multi-layer regression test; perf-guard at 100 layers × 100 nodes still <50 ms). - `pnpm --filter @understand-anything/dashboard exec tsc --noEmit` — clean. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../dashboard/src/components/ExportMenu.tsx | 4 +-- .../packages/dashboard/src/store.ts | 36 +++++++++++++++---- .../src/utils/__tests__/filters.test.ts | 27 ++++++++++++-- .../packages/dashboard/src/utils/filters.ts | 22 +++++++++--- 4 files changed, 72 insertions(+), 17 deletions(-) diff --git a/understand-anything-plugin/packages/dashboard/src/components/ExportMenu.tsx b/understand-anything-plugin/packages/dashboard/src/components/ExportMenu.tsx index fbaa1c3..e3ed709 100644 --- a/understand-anything-plugin/packages/dashboard/src/components/ExportMenu.tsx +++ b/understand-anything-plugin/packages/dashboard/src/components/ExportMenu.tsx @@ -20,7 +20,7 @@ function downloadBlob(blob: Blob, filename: string) { export default function ExportMenu() { const graph = useDashboardStore((s) => s.graph); - const nodeIdToLayerId = useDashboardStore((s) => s.nodeIdToLayerId); + const nodeIdToLayerIds = useDashboardStore((s) => s.nodeIdToLayerIds); const filters = useDashboardStore((s) => s.filters); const exportMenuOpen = useDashboardStore((s) => s.exportMenuOpen); const toggleExportMenu = useDashboardStore((s) => s.toggleExportMenu); @@ -188,7 +188,7 @@ export default function ExportMenu() { ? graph.nodes.filter((n) => !subFileTypes.has(n.type)) : graph.nodes; - filteredGraphNodes = filterNodes(filteredGraphNodes, nodeIdToLayerId, filters); + filteredGraphNodes = filterNodes(filteredGraphNodes, nodeIdToLayerIds, filters); const filteredNodeIds = new Set(filteredGraphNodes.map((n) => n.id)); let filteredGraphEdges = graph.edges.filter( diff --git a/understand-anything-plugin/packages/dashboard/src/store.ts b/understand-anything-plugin/packages/dashboard/src/store.ts index 564e0bd..b635d66 100644 --- a/understand-anything-plugin/packages/dashboard/src/store.ts +++ b/understand-anything-plugin/packages/dashboard/src/store.ts @@ -55,24 +55,42 @@ export type NodeCategory = "code" | "config" | "docs" | "infra" | "data" | "doma * the dashboard reads via store selectors. Centralised so `setGraph` and * any future graph-replacement path stay in sync. * - * `nodeIdToLayerId` preserves the prior `findNodeLayer` "first matching - * layer wins" semantics — if a node id appears in multiple layers (rare - * but legal in the schema), the first occurrence in `graph.layers` order - * is the one we map to. + * Two layer indexes, intentionally distinct: + * + * - `nodeIdToLayerId` preserves the prior `findNodeLayer` "first matching + * layer wins" semantics — if a node id appears in multiple layers + * (rare but legal in the schema), the first occurrence in `graph.layers` + * order is the one we map to. Drives navigation (drillIntoLayer, tour + * step → layer, sidebar history) where a single canonical layer is the + * right answer. + * + * - `nodeIdToLayerIds` records *every* layer a node belongs to. Drives + * membership queries (filterNodes) where the prior `Layer[] + + * layer.nodeIds.includes` shape was any-layer-wins — a node in L1 and + * L2 with only L2 selected must still pass. Collapsing to first-wins + * for filtering would be a silent regression. */ function buildGraphIndexes(graph: KnowledgeGraph): { nodesById: Map; nodeIdToLayerId: Map; + nodeIdToLayerIds: Map>; } { const nodesById = new Map(); for (const node of graph.nodes) nodesById.set(node.id, node); const nodeIdToLayerId = new Map(); + const nodeIdToLayerIds = new Map>(); for (const layer of graph.layers) { for (const nid of layer.nodeIds) { if (!nodeIdToLayerId.has(nid)) nodeIdToLayerId.set(nid, layer.id); + let set = nodeIdToLayerIds.get(nid); + if (!set) { + set = new Set(); + nodeIdToLayerIds.set(nid, set); + } + set.add(layer.id); } } - return { nodesById, nodeIdToLayerId }; + return { nodesById, nodeIdToLayerId, nodeIdToLayerIds }; } /** Maximum number of entries in the sidebar navigation history. */ @@ -82,8 +100,10 @@ interface DashboardStore { graph: KnowledgeGraph | null; /** id → node lookup, rebuilt by setGraph. Empty before any graph loads. */ nodesById: Map; - /** id → layer id, rebuilt by setGraph. Empty before any graph loads. */ + /** id → layer id (first-matching-layer wins), rebuilt by setGraph. Empty before any graph loads. */ nodeIdToLayerId: Map; + /** id → set of every layer the node belongs to, rebuilt by setGraph. Empty before any graph loads. */ + nodeIdToLayerIds: Map>; selectedNodeId: string | null; searchQuery: string; searchResults: SearchResult[]; @@ -229,6 +249,7 @@ export const useDashboardStore = create()((set, get) => ({ graph: null, nodesById: new Map(), nodeIdToLayerId: new Map(), + nodeIdToLayerIds: new Map>(), selectedNodeId: null, searchQuery: "", searchResults: [], @@ -283,11 +304,12 @@ export const useDashboardStore = create()((set, get) => ({ const { viewMode, domainGraph, activeDomainId } = get(); // Preserve domain view if a domain graph is already loaded const keepDomainView = viewMode === "domain" && domainGraph !== null; - const { nodesById, nodeIdToLayerId } = buildGraphIndexes(graph); + const { nodesById, nodeIdToLayerId, nodeIdToLayerIds } = buildGraphIndexes(graph); set({ graph, nodesById, nodeIdToLayerId, + nodeIdToLayerIds, searchEngine, searchResults, navigationLevel: "overview", diff --git a/understand-anything-plugin/packages/dashboard/src/utils/__tests__/filters.test.ts b/understand-anything-plugin/packages/dashboard/src/utils/__tests__/filters.test.ts index 15c4abf..189b223 100644 --- a/understand-anything-plugin/packages/dashboard/src/utils/__tests__/filters.test.ts +++ b/understand-anything-plugin/packages/dashboard/src/utils/__tests__/filters.test.ts @@ -46,11 +46,16 @@ function defaultFilters(overrides: Partial = {}): FilterState { }; } -function indexLayers(layers: Layer[]): Map { - const m = new Map(); +function indexLayers(layers: Layer[]): Map> { + const m = new Map>(); for (const l of layers) { for (const nid of l.nodeIds) { - if (!m.has(nid)) m.set(nid, l.id); + let set = m.get(nid); + if (!set) { + set = new Set(); + m.set(nid, set); + } + set.add(l.id); } } return m; @@ -102,6 +107,22 @@ describe("filterNodes", () => { expect(out.map((n) => n.id)).toEqual(["a"]); }); + it("keeps a multi-layer node when any of its layers is selected (any-layer-wins)", () => { + // Regression for the silent first-wins behavior change in #112: a node + // X listed in both L1 (declared first) and L2, with only L2 selected, + // must still pass — matching the prior `layers.some(...)` shape. The + // first-wins `nodeIdToLayerId` index that drives navigation would + // have dropped X here. + const nodes = [node("x"), node("y")]; + const idx = indexLayers([ + { id: "L1", name: "L1", description: "", nodeIds: ["x"] }, + { id: "L2", name: "L2", description: "", nodeIds: ["x", "y"] }, + ]); + const filters = defaultFilters({ layerIds: new Set(["L2"]) }); + const out = filterNodes(nodes, idx, filters); + expect(out.map((n) => n.id).sort()).toEqual(["x", "y"]); + }); + it("ignores layer filter when no layers are selected (parity with prior shape)", () => { const nodes = [node("a"), node("orphan")]; // idx maps "a"; "orphan" isn't in any layer. With layer filter empty, diff --git a/understand-anything-plugin/packages/dashboard/src/utils/filters.ts b/understand-anything-plugin/packages/dashboard/src/utils/filters.ts index d0d699d..eef3cc2 100644 --- a/understand-anything-plugin/packages/dashboard/src/utils/filters.ts +++ b/understand-anything-plugin/packages/dashboard/src/utils/filters.ts @@ -5,14 +5,20 @@ import { EDGE_CATEGORY_MAP } from "../store"; /** * Filter nodes based on active filters. * - * Pass `nodeIdToLayerId` from the store (precomputed once on `setGraph`) + * Pass `nodeIdToLayerIds` from the store (precomputed once on `setGraph`) * so the layer-membership check is O(1) per node. The previous shape took * `Layer[]` and ran `layer.nodeIds.includes(node.id)` per node-per-layer, * which was O(N × L × K) and dominated export time on large graphs (#102). + * + * Membership semantics are any-layer-wins, matching the prior shape: a + * node in L1 and L2 with only L2 selected passes. The store's other + * index, `nodeIdToLayerId`, is first-wins and is for navigation, not + * filtering — using it here would silently drop multi-layer nodes whose + * first declared layer isn't selected. */ export function filterNodes( nodes: GraphNode[], - nodeIdToLayerId: Map, + nodeIdToLayerIds: Map>, filters: FilterState, ): GraphNode[] { const hasLayerFilter = filters.layerIds.size > 0; @@ -29,10 +35,16 @@ export function filterNodes( // Filter by layer (if any layers are selected) if (hasLayerFilter) { - const layerId = nodeIdToLayerId.get(node.id); - if (!layerId || !filters.layerIds.has(layerId)) { - return false; + const layerIds = nodeIdToLayerIds.get(node.id); + if (!layerIds) return false; + let inSelected = false; + for (const lid of layerIds) { + if (filters.layerIds.has(lid)) { + inSelected = true; + break; + } } + if (!inSelected) return false; } return true;