mirror of
https://github.com/Egonex-AI/Understand-Anything.git
synced 2026-06-22 10:58:03 +08:00
fix(dashboard): preserve any-layer-wins membership for filterNodes
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<string, string>` — first-matching-layer wins. Drives navigation (drillIntoLayer / tour step → layer / sidebar history) where one canonical layer is the right answer. Unchanged. - `nodeIdToLayerIds: Map<string, Set<string>>` — 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) <noreply@anthropic.com>
This commit is contained in:
@@ -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(
|
||||
|
||||
@@ -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<string, GraphNode>;
|
||||
nodeIdToLayerId: Map<string, string>;
|
||||
nodeIdToLayerIds: Map<string, Set<string>>;
|
||||
} {
|
||||
const nodesById = new Map<string, GraphNode>();
|
||||
for (const node of graph.nodes) nodesById.set(node.id, node);
|
||||
const nodeIdToLayerId = new Map<string, string>();
|
||||
const nodeIdToLayerIds = new Map<string, Set<string>>();
|
||||
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<string>();
|
||||
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<string, GraphNode>;
|
||||
/** 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<string, string>;
|
||||
/** id → set of every layer the node belongs to, rebuilt by setGraph. Empty before any graph loads. */
|
||||
nodeIdToLayerIds: Map<string, Set<string>>;
|
||||
selectedNodeId: string | null;
|
||||
searchQuery: string;
|
||||
searchResults: SearchResult[];
|
||||
@@ -229,6 +249,7 @@ export const useDashboardStore = create<DashboardStore>()((set, get) => ({
|
||||
graph: null,
|
||||
nodesById: new Map<string, GraphNode>(),
|
||||
nodeIdToLayerId: new Map<string, string>(),
|
||||
nodeIdToLayerIds: new Map<string, Set<string>>(),
|
||||
selectedNodeId: null,
|
||||
searchQuery: "",
|
||||
searchResults: [],
|
||||
@@ -283,11 +304,12 @@ export const useDashboardStore = create<DashboardStore>()((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",
|
||||
|
||||
@@ -46,11 +46,16 @@ function defaultFilters(overrides: Partial<FilterState> = {}): FilterState {
|
||||
};
|
||||
}
|
||||
|
||||
function indexLayers(layers: Layer[]): Map<string, string> {
|
||||
const m = new Map<string, string>();
|
||||
function indexLayers(layers: Layer[]): Map<string, Set<string>> {
|
||||
const m = new Map<string, Set<string>>();
|
||||
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<string>();
|
||||
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,
|
||||
|
||||
@@ -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<string, string>,
|
||||
nodeIdToLayerIds: Map<string, Set<string>>,
|
||||
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;
|
||||
|
||||
Reference in New Issue
Block a user