diff --git a/packages/graph-explorer/src/components/VertexRow.tsx b/packages/graph-explorer/src/components/VertexRow.tsx index f6b7fbc48..8a81b61f8 100644 --- a/packages/graph-explorer/src/components/VertexRow.tsx +++ b/packages/graph-explorer/src/components/VertexRow.tsx @@ -1,6 +1,6 @@ import type { ComponentPropsWithoutRef } from "react"; -import { type DisplayVertex, useVertexStyle } from "@/core"; +import { type DisplayVertex, useVertexStyleForTypes } from "@/core"; import { ASCII, cn, LABELS } from "@/utils"; import { SearchResultSubtitle, SearchResultTitle, VertexSymbol } from "."; @@ -14,7 +14,7 @@ export function VertexRow({ vertex: DisplayVertex; name?: string; } & ComponentPropsWithoutRef<"div">) { - const vertexStyle = useVertexStyle(vertex.primaryType); + const vertexStyle = useVertexStyleForTypes(vertex.types); const resultName = name ? `${name}: ` : ""; const nameIsSameAsTypes = vertex.displayTypes === vertex.displayName; const isDefaultType = vertex.displayTypes === LABELS.MISSING_TYPE; diff --git a/packages/graph-explorer/src/core/StateProvider/displayVertex.ts b/packages/graph-explorer/src/core/StateProvider/displayVertex.ts index 5ca4560eb..b40b5ad95 100644 --- a/packages/graph-explorer/src/core/StateProvider/displayVertex.ts +++ b/packages/graph-explorer/src/core/StateProvider/displayVertex.ts @@ -9,7 +9,9 @@ import { nodeSelector, nodesSelectedIdsAtom, queryEngineSelector, + resolveVertexStyleForTypes, useVertex, + userVertexStylesAtom, type Vertex, type VertexId, vertexStyleByTypeAtom, @@ -108,7 +110,14 @@ const displayVertexSelector = atomFamily((vertex: Vertex) => return LABELS.MISSING_VALUE; } - const vertexStyle = get(vertexStyleByTypeAtom(vertex.type)); + // Merged across every type the vertex has, not just `vertex.type`/`primaryType` — + // a resource asserting multiple rdf:type values (e.g. under RDFS/OWL inference, + // where every superclass becomes a peer rdf:type) should pick up styling and the + // displayNameAttribute from whichever of its types set them, not an arbitrary one. + const vertexStyle = resolveVertexStyleForTypes( + vertexTypes, + get(userVertexStylesAtom), + ); const displayName = getDisplayAttributeValueByName( vertexStyle.displayNameAttribute, ); diff --git a/packages/graph-explorer/src/core/StateProvider/graphStyles.test.ts b/packages/graph-explorer/src/core/StateProvider/graphStyles.test.ts index 0957a2df8..febeed118 100644 --- a/packages/graph-explorer/src/core/StateProvider/graphStyles.test.ts +++ b/packages/graph-explorer/src/core/StateProvider/graphStyles.test.ts @@ -5,15 +5,19 @@ import { act } from "react"; import { createEdgeType, createVertexType } from "@/core"; import { DbState, renderHookWithState } from "@/utils/testing"; +import type { VertexType } from "../entities"; + import { appDefaultEdgeStyle, appDefaultVertexStyle, edgeStyleAtom, type EdgeStyleStorage, + mergeVertexStyleFields, useEdgeStyling, useVertexStyling, vertexStyleAtom, type VertexStyleStorage, + vertexTypeSetKey, } from "./graphStyles"; function createExpectedVertex(existing: VertexStyleStorage) { @@ -460,3 +464,115 @@ describe("edgeStyleAtom", () => { ); }); }); + +describe("vertexTypeSetKey", () => { + it("is stable regardless of the input order", () => { + const a = createVertexType("A"); + const b = createVertexType("B"); + const c = createVertexType("C"); + + expect(vertexTypeSetKey([a, b, c])).toBe(vertexTypeSetKey([c, a, b])); + }); + + it("ignores duplicate types", () => { + const a = createVertexType("A"); + const b = createVertexType("B"); + + expect(vertexTypeSetKey([a, b, a])).toBe(vertexTypeSetKey([a, b])); + }); + + it("differs for different type sets", () => { + const a = createVertexType("A"); + const b = createVertexType("B"); + + expect(vertexTypeSetKey([a])).not.toBe(vertexTypeSetKey([a, b])); + }); +}); + +describe("mergeVertexStyleFields", () => { + it("merges non-conflicting fields from every type", () => { + const equipment = createVertexType("Equipment"); + const breaker = createVertexType("Breaker"); + const userStyles = new Map([ + [equipment, { type: equipment, borderColor: "black", shape: "hexagon" }], + [breaker, { type: breaker, color: "red", iconUrl: "lucide:zap-off" }], + ]); + + expect( + mergeVertexStyleFields([breaker, equipment], userStyles), + ).toStrictEqual({ + borderColor: "black", + shape: "hexagon", + color: "red", + iconUrl: "lucide:zap-off", + }); + }); + + it("resolves a field set by more than one type by lexicographic order, last wins", () => { + const a = createVertexType("A"); + const z = createVertexType("Z"); + const userStyles = new Map([ + [a, { type: a, color: "from-a" }], + [z, { type: z, color: "from-z" }], + ]); + + // Order of the input array must not matter — only the type names' sort order does. + expect(mergeVertexStyleFields([a, z], userStyles).color).toBe("from-z"); + expect(mergeVertexStyleFields([z, a], userStyles).color).toBe("from-z"); + }); + + it("skips types with no stored style", () => { + const styled = createVertexType("Styled"); + const unstyled = createVertexType("Unstyled"); + const userStyles = new Map([ + [styled, { type: styled, color: "red" }], + ]); + + expect( + mergeVertexStyleFields([styled, unstyled], userStyles), + ).toStrictEqual({ color: "red" }); + }); + + it("returns an empty object when no type has a stored style", () => { + const a = createVertexType("A"); + expect(mergeVertexStyleFields([a], new Map())).toStrictEqual({}); + }); +}); + +describe("vertexStyleAtom.getForTypes", () => { + it("merges styles across all of a vertex's types, overlaid on defaults", () => { + const dbState = new DbState(); + const equipment = createVertexType("Equipment"); + const breaker = createVertexType("Breaker"); + dbState.addVertexStyle(equipment, { borderColor: "black" }); + dbState.addVertexStyle(breaker, { color: "red" }); + + const { result } = renderHookWithState( + () => useAtomValue(vertexStyleAtom), + dbState, + ); + + const resolved = result.current.getForTypes([breaker, equipment]); + expect(resolved.color).toBe("red"); + expect(resolved.borderColor).toBe("black"); + // Every other field still falls back to the app default. + expect(resolved.shape).toBe(appDefaultVertexStyle.shape); + }); + + it("is order-independent — the same two types resolve identically regardless of array order", () => { + const dbState = new DbState(); + const a = createVertexType("A"); + const z = createVertexType("Z"); + dbState.addVertexStyle(a, { color: "from-a" }); + dbState.addVertexStyle(z, { color: "from-z" }); + + const { result } = renderHookWithState( + () => useAtomValue(vertexStyleAtom), + dbState, + ); + + expect(result.current.getForTypes([a, z])).toStrictEqual( + result.current.getForTypes([z, a]), + ); + }); +}); diff --git a/packages/graph-explorer/src/core/StateProvider/graphStyles.ts b/packages/graph-explorer/src/core/StateProvider/graphStyles.ts index 8026c7a37..51901b2b0 100644 --- a/packages/graph-explorer/src/core/StateProvider/graphStyles.ts +++ b/packages/graph-explorer/src/core/StateProvider/graphStyles.ts @@ -206,6 +206,15 @@ export const vertexStyleAtom = atom(get => { get(type: VertexType) { return resolveVertexStyle(type, userStyles.get(type)); }, + /** + * The resolved style for a vertex carrying one or more types — see + * {@link resolveVertexStyleForTypes}. Use this (not repeated `get` calls) + * whenever the caller has an actual vertex instance rather than a single + * schema type, so styling reflects every type the vertex has. + */ + getForTypes(types: readonly VertexType[]) { + return resolveVertexStyleForTypes(types, userStyles); + }, }; }); @@ -231,6 +240,83 @@ export function resolveVertexStyle( } as const; } +/** + * A stable, order-independent identity for a vertex's full set of types — + * every distinct combination of types gets one key, and the same set of + * types (in any order) always produces the same key. Reuses the + * {@link VertexType} brand since it is consumed exactly like one: as an + * opaque Cytoscape selector value and style-lookup key (see + * `useGraphStyles.ts` and {@link resolveVertexStyleForTypes}). It is never a + * real schema type and must not be shown to the user or looked up against + * schema/connection data — {@link DisplayVertex.displayTypes} is the + * user-facing type label. + */ +function sortedUniqueTypes(types: readonly VertexType[]): VertexType[] { + return [...new Set(types)].sort(); +} + +export function vertexTypeSetKey(types: readonly VertexType[]): VertexType { + return sortedUniqueTypes(types).join(" ") as VertexType; +} + +/** + * Merges the stored user style for every one of a vertex's types into one set + * of fields, so a vertex with multiple `rdf:type` values (common once RDFS/OWL + * inference is in play — every superclass ends up asserted as a peer + * `rdf:type`) picks up styling from all of them rather than an arbitrary one. + * A field left unset by one type falls through to another type that does set + * it — e.g. a `borderColor` styled once on a shared ancestor class applies to + * every subtype automatically, without repeating it on each one. + * + * Conflicts — two of the vertex's types both set the same field — are + * resolved by folding the types in ascending lexicographic order, so the + * lexicographically-last type's value wins. This is a **stable, reproducible + * tiebreak, not a specificity judgement**: nothing in the app tracks + * `rdfs:subClassOf` (or any other) class hierarchy today — schema sync only + * samples instance data (types + attribute names) for every connector, so + * there is no signal available to determine which of a resource's asserted + * types is actually "more specific" than another. Sorting at least makes the + * outcome a deterministic property of the type names themselves instead of an + * accident of SPARQL query result order, which is what motivated this fix + * (see the linked issue). A hierarchy-aware tiebreak would be a natural + * follow-up once class-hierarchy data is tracked anywhere in the app. + */ +export function mergeVertexStyleFields( + types: readonly VertexType[], + userStyles: ReadonlyMap, +): Omit { + let merged: Omit = {}; + for (const type of sortedUniqueTypes(types)) { + const style = userStyles.get(type); + if (!style) { + continue; + } + // eslint-disable-next-line @typescript-eslint/no-unused-vars -- drop the per-type `type` field, only the fields matter + const { type: _type, ...fields } = style; + merged = { ...merged, ...fields }; + } + return merged; +} + +/** + * The resolved style for a vertex carrying one or more types — see + * {@link mergeVertexStyleFields} for how conflicts across types are resolved. + * `type` on the result is {@link vertexTypeSetKey}'s composite key, not a + * single real type, since the resolved style may draw fields from more than + * one type. + */ +export function resolveVertexStyleForTypes( + types: readonly VertexType[], + userStyles: ReadonlyMap, +): VertexStyle { + const merged = mergeVertexStyleFields(types, userStyles); + return { + type: vertexTypeSetKey(types), + ...appDefaultVertexStyle, + ...merged, + } as const; +} + /** The user's edge style overlaid on the app defaults. */ export function resolveEdgeStyle( type: EdgeType, @@ -272,6 +358,22 @@ export function useVertexStyle(type: VertexType): VertexStyle { return useDeferredValue(useAtomValue(vertexStyleByTypeAtom(type))); } +/** + * Returns the resolved style for a vertex instance, merged across every type + * it has — see {@link resolveVertexStyleForTypes}. Use this instead of + * {@link useVertexStyle} whenever `types` is an actual vertex's asserted + * types rather than a single schema type being styled/edited on its own + * (e.g. the Schema view's Styles panel, or a legend listing every known + * type — those still want {@link useVertexStyle} for one type at a time). + */ +export function useVertexStyleForTypes( + types: readonly VertexType[], +): VertexStyle { + return useDeferredValue( + useAtomValue(vertexStyleByTypesAtom(vertexTypeSetKey(types))), + ); +} + /** Returns the resolved style for the specified edge type. */ export function useEdgeStyle(type: EdgeType): EdgeStyle { return useDeferredValue(useAtomValue(edgeStyleByTypeAtom(type))); @@ -284,6 +386,20 @@ export const vertexStyleByTypeAtom = atomFamily((type: VertexType) => atom(get => get(vertexStyleAtom).get(type)), ); +/** + * Returns the resolved style for a vertex's full set of types, keyed by + * {@link vertexTypeSetKey} — atomFamily needs a primitive key, and the key + * already carries the sorted, deduplicated type list, so it is split back + * into types rather than threading the original array through separately. + */ +export const vertexStyleByTypesAtom = atomFamily((typesKey: VertexType) => + atom(get => + get(vertexStyleAtom).getForTypes( + typesKey === "" ? [] : (typesKey.split(" ") as VertexType[]), + ), + ), +); + /** * Returns the resolved style for the specified edge type. */ diff --git a/packages/graph-explorer/src/core/StateProvider/renderedEntities.ts b/packages/graph-explorer/src/core/StateProvider/renderedEntities.ts index d08814f5e..e864f725c 100644 --- a/packages/graph-explorer/src/core/StateProvider/renderedEntities.ts +++ b/packages/graph-explorer/src/core/StateProvider/renderedEntities.ts @@ -13,6 +13,7 @@ import { useAllNeighbors, useDisplayEdgesInCanvas, useDisplayVerticesInCanvas, + vertexTypeSetKey, type VertexId, } from "@/core"; @@ -170,7 +171,10 @@ function createRenderedVertex(vertex: DisplayVertex, neighborCount: number) { return { data: { id: createRenderedVertexId(vertex.id), - type: vertex.primaryType, + // The Cytoscape stylesheet selector key — see useGraphStyles.ts. Covers every + // type the vertex has (not just primaryType) so a multi-typed vertex's rule + // is built from all of them; see resolveVertexStyleForTypes for why. + type: vertexTypeSetKey(vertex.types), vertexId: vertex.id, displayName: vertex.displayName, displayTypes: vertex.displayTypes, diff --git a/packages/graph-explorer/src/modules/GraphViewer/useGraphStyles.test.tsx b/packages/graph-explorer/src/modules/GraphViewer/useGraphStyles.test.tsx index 33da0ec8a..1aa58652b 100644 --- a/packages/graph-explorer/src/modules/GraphViewer/useGraphStyles.test.tsx +++ b/packages/graph-explorer/src/modules/GraphViewer/useGraphStyles.test.tsx @@ -1,18 +1,27 @@ // @vitest-environment happy-dom -import { waitFor } from "@testing-library/react"; +import { act, waitFor } from "@testing-library/react"; import { beforeEach, describe, expect, it } from "vitest"; import type { GraphProps } from "@/components/Graph"; -import { createEdgeType, createVertexType } from "@/core"; +import { + type AppStore, + createVertex, + createEdgeType, + createVertexType, + nodesAtom, + toNodeMap, + userVertexStylesAtom, +} from "@/core"; import { createRandomEdgeTypeConfig, createRandomVertexTypeConfig, DbState, + renderHookWithJotai, renderHookWithState, } from "@/utils/testing"; -import useGraphStyles from "./useGraphStyles"; +import useGraphStyles, { useAllRenderedVertexStyles } from "./useGraphStyles"; // A raster icon resolves synchronously to its url, so this test can exercise // the real icon pipeline and still pin the expected background image. @@ -349,4 +358,109 @@ describe("useGraphStyles", () => { ).toBeDefined(); expect(getStyles(result)[`edge[type="${edgeConfig.type}"]`]).toBeDefined(); }); + + // Regression test for a real infinite render loop this hook caused in + // manual testing (multi-typed vertex + a fresh mount/session restore). + // + // Root cause: `useAllRenderedVertexStyles` used to source its vertex list + // via `useDisplayVerticesInCanvas()`, whose underlying selector + // (`displayVerticesInCanvasSelector`) allocates a brand-new array/Map on + // every single read (`get(nodesAtom).values().toArray()` feeding a + // reference-keyed atomFamily), even when `nodesAtom` itself hasn't changed. + // The hook's `useMemo` therefore never actually memoized, so every render + // produced a "new" config array by identity — which, chained through + // `useGraphStyles`'s `useDeferredValue`, never converged and pegged the CPU + // in a real browser tab. The fix reads `nodesAtom` directly instead, which + // Jotai only changes identity for when it's actually written to. + // + // This is asserted directly on the memoized value's referential stability, + // not via a render-count/timing probe: an isolated `renderHook` test of + // `useGraphStyles` alone did not reproduce the actual hang even on the + // unfixed code — the real feedback loop needs the mounted `` + // component's Cytoscape-sync effects (which aren't exercised here) to close + // the cycle. Referential stability of the memoized array is the precise, + // deterministic property whose violation caused the bug, and is what any + // future change here must preserve regardless of how it's wired downstream. + it("returns the same array reference across re-renders when nothing changed", () => { + const typeA = createVertexType("TypeA"); + const typeB = createVertexType("TypeB"); + dbState.addVertexStyle(typeA, { color: "red" }); + dbState.addVertexStyle(typeB, { borderColor: "blue" }); + dbState.addVertexToGraph(createVertex({ id: "v1", types: [typeA, typeB] })); + + const { result, rerender } = renderHookWithState( + () => useAllRenderedVertexStyles(), + dbState, + ); + + const firstRender = result.current; + expect(firstRender.length).toBe(1); + + act(() => rerender()); + + expect(result.current).toBe(firstRender); + }); + + // A stronger version of the regression above, closer to the actual reported + // scenario: a session restore adds vertices one at a time (a fresh + // `nodesAtom` identity per vertex), most of them reusing a type combination + // already on the canvas. If this hook rebuilds its array on every one of + // those adds — not just when the *set of distinct combinations* changes — + // it forces a full Cytoscape stylesheet rebuild per vertex, which for a + // large restored graph is what actually produced the reported hang (not a + // literal non-terminating loop, but indistinguishable from one). + it("does not rebuild the resolved styles when a new vertex reuses an existing type combination", () => { + const typeA = createVertexType("TypeA"); + let store!: AppStore; + + const { result } = renderHookWithJotai( + () => useAllRenderedVertexStyles(), + s => { + store = s; + store.set( + userVertexStylesAtom, + new Map([[typeA, { type: typeA, color: "red" }]]), + ); + store.set( + nodesAtom, + toNodeMap([createVertex({ id: "v1", types: [typeA] })]), + ); + }, + ); + + const firstRender = result.current; + expect(firstRender.length).toBe(1); + + // Simulate the next step of a session restore: one more vertex of the + // exact same type combination, via a brand-new nodesAtom identity (the + // same shape as a real incremental restore). + act(() => { + store.set( + nodesAtom, + toNodeMap([ + createVertex({ id: "v1", types: [typeA] }), + createVertex({ id: "v2", types: [typeA] }), + ]), + ); + }); + + expect(result.current).toBe(firstRender); + + // A vertex that introduces a genuinely new combination must still produce + // a new (correct) result — this isn't just permanently stuck on the first + // value. + const typeB = createVertexType("TypeB"); + act(() => { + store.set( + nodesAtom, + toNodeMap([ + createVertex({ id: "v1", types: [typeA] }), + createVertex({ id: "v3", types: [typeB] }), + ]), + ); + }); + + expect(result.current).not.toBe(firstRender); + expect(result.current.length).toBe(2); + }); }); diff --git a/packages/graph-explorer/src/modules/GraphViewer/useGraphStyles.ts b/packages/graph-explorer/src/modules/GraphViewer/useGraphStyles.ts index 869ded042..1179eb614 100644 --- a/packages/graph-explorer/src/modules/GraphViewer/useGraphStyles.ts +++ b/packages/graph-explorer/src/modules/GraphViewer/useGraphStyles.ts @@ -1,12 +1,17 @@ import Color from "color"; -import { useDeferredValue } from "react"; +import { useAtomValue } from "jotai"; +import { useDeferredValue, useMemo } from "react"; import type { GraphProps } from "@/components/Graph"; import { + nodesAtom, type EdgeStyle, + resolveVertexStyleForTypes, useAllEdgeStyles, useAllVertexStyles, + userVertexStylesAtom, + vertexTypeSetKey, type VertexStyle, type VertexType, } from "@/core"; @@ -19,20 +24,91 @@ const LINE_PATTERN = { dotted: [1, 2], }; +/** + * One resolved style per distinct combination of types among the vertices + * currently on the canvas, in addition to (not instead of) the one-per-schema- + * type styles from `useAllVertexStyles`. A vertex can carry more than one type + * (e.g. under RDFS/OWL inference, where every superclass is asserted as a + * peer rdf:type), and its Cytoscape node is tagged with the combined key from + * `vertexTypeSetKey` (see `renderedEntities.ts`) rather than a single type, so + * a merged rule is needed for that key too — see `resolveVertexStyleForTypes` + * for how conflicting fields across a vertex's types are resolved. For a + * vertex with exactly one type, `vertexTypeSetKey` returns that same type, so + * this produces a redundant entry rather than a wrong one. + * + * Reads `nodesAtom` directly rather than `useDisplayVerticesInCanvas()` — + * only `.types` is needed here, and `displayVerticesInCanvasSelector` + * rebuilds its array/Map on every single read (`get(nodesAtom).values().toArray()` + * allocates fresh, and its atomFamily is keyed by reference), so memoizing + * against its output never actually memoizes. `nodesAtom` itself only + * changes identity when actually written to, so memoizing against it is + * real memoization. + * + * That alone isn't enough, though: `nodesAtom` gets a new identity on + * *every* vertex add/remove (e.g. once per vertex while a session restores), + * and most of those adds don't introduce a new type combination — they add + * another vertex of a combination already on the canvas. Memoizing directly + * against `nodes` would still rebuild this array (and, downstream, the whole + * Cytoscape stylesheet — expensive, since it re-resolves icons per type) on + * every single one of those adds. Deriving a stable, content-based key from + * the *distinct* type combinations first — a plain string, compared by + * value — means the expensive rebuild only happens when the actual set of + * combinations changes, not on every vertex added to an existing one. This + * is what a fresh session restore with many vertices needs: without it, the + * combination of "new nodesAtom identity per vertex" and "this hook's output + * feeding back into a re-rendered `styles` prop" is a per-vertex full + * stylesheet rebuild — pathological for a large restored graph, not a true + * non-terminating loop, but indistinguishable from one in practice. + * + * Exported (only) so a test can assert directly on that stability — see + * `useGraphStyles.test.tsx` for why a render-count assertion alone doesn't + * reliably catch a regression here. + */ +export function useAllRenderedVertexStyles(): VertexStyle[] { + const nodes = useAtomValue(nodesAtom); + const userStyles = useAtomValue(userVertexStylesAtom); + + const distinctTypeSetKeys = useMemo(() => { + const keys = new Set(); + for (const vertex of nodes.values()) { + keys.add(vertexTypeSetKey(vertex.types)); + } + return [...keys].sort().join("\n"); + }, [nodes]); + + return useMemo(() => { + if (distinctTypeSetKeys === "") { + return []; + } + // Each key is itself a space-joined list of the combination's individual + // types (see `vertexTypeSetKey`) — split back into a types array so + // `resolveVertexStyleForTypes` looks up each *individual* type's stored + // style, not the composite key as if it were one type's name. + return distinctTypeSetKeys + .split("\n") + .map(key => + resolveVertexStyleForTypes(key.split(" ") as VertexType[], userStyles), + ); + }, [distinctTypeSetKeys, userStyles]); +} + export default function useGraphStyles() { const vtConfigs = useAllVertexStyles(); + const renderedVtConfigs = useAllRenderedVertexStyles(); const etConfigs = useAllEdgeStyles(); const deferredVtConfigs = useDeferredValue(vtConfigs); + const deferredRenderedVtConfigs = useDeferredValue(renderedVtConfigs); const deferredEtConfigs = useDeferredValue(etConfigs); - const backgroundImageMap = useBackgroundImageMap(deferredVtConfigs); - - return createGraphStyles( - deferredVtConfigs, - deferredEtConfigs, - backgroundImageMap, + const allVtConfigs = useMemo( + () => [...deferredVtConfigs, ...deferredRenderedVtConfigs], + [deferredVtConfigs, deferredRenderedVtConfigs], ); + + const backgroundImageMap = useBackgroundImageMap(allVtConfigs); + + return createGraphStyles(allVtConfigs, deferredEtConfigs, backgroundImageMap); } function createGraphStyles(