diff --git a/docs/adr/20260813-element-data-style-mappers.md b/docs/adr/20260813-element-data-style-mappers.md index 203efcc67..f4eb4884e 100644 --- a/docs/adr/20260813-element-data-style-mappers.md +++ b/docs/adr/20260813-element-data-style-mappers.md @@ -12,7 +12,7 @@ The graph canvas colours each vertex/edge by its type. The obvious Cytoscape idi Precompute each element's resolved style values onto its Cytoscape `ele.data()` as `ge_*` fields (`vertexStyleData` / `edgeStyleData` in `core/StateProvider/graphElementStyleData.ts`) at the element-construction seams (`renderedEntities.ts` for the explorer graph, `useSchemaGraphData.ts` for the schema view), and read them back through a single `node` rule and single `edge` rule using Cytoscape `data(…)` mappers (`CANVAS_STYLES` in `useGraphStyles.ts`). The stylesheet is now O(1) in the number of types. -Two structured/optional properties can't be a plain always-present mapper, so they keep a gated selector that applies only when the field exists: `node[__iconUrl]` (background image; the field is omitted when a type has no icon) and `edge[ge_lineDashPattern]` (omitted for solid lines). The dotted→dashed remap, the dash-pattern lookup, the border-opacity derivation, and the `isDark` label-text-colour pick are baked into the producer functions so the stylesheet stays pure `data(…)`. +**Every field is always set, so there are no gated selectors.** `cy.json({ elements })` _merges_ element data — `ele.data(obj)` adds and overwrites keys but never deletes ones missing from the new object. A sometimes-absent field therefore can never be cleared once applied, stranding a stale value on an already-drawn element: an icon that stopped resolving kept rendering the previous image. So `ge_iconUrl` carries `"none"` when a type has no icon, and `ge_lineDashPattern` carries cytoscape's default for solid lines, letting both live on the base `node` / `edge` rule. The dotted→dashed remap, the dash-pattern lookup, the border-opacity derivation, and the `isDark` label-text-colour pick are baked into the producer functions so the stylesheet stays pure `data(…)`. ## Consequences @@ -21,3 +21,4 @@ Two structured/optional properties can't be a plain always-present mapper, so th - **A stylesheet consumer must merge into the base `node`/`edge` rules, never replace them.** `useSchemaGraphStyles` adds a schema label by spreading `{ ...baseStyles.node, label: … }`; overwriting the `node`/`edge` keys wholesale would discard every `ge_*` mapper and render the graph unstyled. Guarded by `useSchemaGraphStyles.test.tsx`. - Context-count regression guards (`useGraphStyles.contextCount.test.tsx`, `useSchemaGraphStyles.test.tsx`) assert the selector count stays O(1) regardless of type count, so the per-type-selector approach can't creep back in unnoticed. - A future reader seeing `data(ge_*)` mappers and no per-type selectors should not "restore" per-type selectors — that is the exact regression this avoids. +- **The `ge_*` producers must be fed a type set scoped to what is drawn, not the whole schema.** Moving styling into element data moves the per-type cost from the stylesheet into the render path, so resolving a style and an icon for every schema type became the new bottleneck (10,044 types resolved to draw 3). Each surface scopes itself to what it draws, and pairs the style with the element so the drawn set and the styled set are the same set by construction — the canvas from `canvasVerticesAtom`, the schema view from its vertex type configs. Do not scope either from a separately-read schema snapshot: `useActiveSchema` is deferred while the type configs are not, so the two can disagree mid-sync and leave an element styleless. Edges have no canvas-scoped equivalent on purpose — edge style data needs no icon resolution, so scoping would buy nothing. diff --git a/docs/agents/react.md b/docs/agents/react.md index 62984c288..c988841aa 100644 --- a/docs/agents/react.md +++ b/docs/agents/react.md @@ -2,6 +2,7 @@ - This project uses React 19 - The React Compiler is enabled — it auto-memoizes components and hooks, so manual `useMemo`, `useCallback`, and `React.memo` are unnecessary in most cases and should be avoided unless profiling shows a specific need +- When per-call memoization genuinely is needed, a hook must not build and return a closure that mutates its own captured cache — the `react-compiler/*` lint rules reject it. Put the cache in a plain `create*` factory and have the hook call it, as `core/StateProvider/styleDataResolvers.ts` does. The two-line hook is not a pointless wrapper: collapsing it back inline fails lint. The factory also ends up store-free and directly unit-testable. - Official React docs: https://react.dev ## General @@ -16,6 +17,11 @@ - Server state goes in TanStack Query - **Exception: vertex icons.** They resolve through the `core/icons/` registry, read via `useSyncExternalStore`, because a per-hook subscription scaled with vertex-type count and locked up the schema view at 10k types. Don't move icon resolution back into TanStack Query — see `docs/adr/20260813-icon-registry-not-react-query.md` +## Client state (Jotai) + +- When a derivation is consumed by more than one hook or pipeline, define it as a derived atom so the store computes it once, rather than a hook each caller re-runs. `visibleVertexIdsAtom` is shared by `useRenderedVertices` and `useRenderedEdges` for exactly this reason; as a hook it ran the filter loop once per call site +- `atomFamily` never evicts unless you call `remove`/`setShouldRemove`, and nothing in this codebase does. Key a family on a stable branded ID, never on a freshly allocated object or array — the latter interns a new entry per recomputation that can never be reached again + ## Feature Modules - Feature modules in `src/modules/` contain all related components, hooks, and utilities diff --git a/packages/graph-explorer/src/components/ColorPopover.tsx b/packages/graph-explorer/src/components/ColorPopover.tsx index d0cc93e8c..9f732227c 100644 --- a/packages/graph-explorer/src/components/ColorPopover.tsx +++ b/packages/graph-explorer/src/components/ColorPopover.tsx @@ -1,6 +1,10 @@ -import type { ComponentPropsWithRef, CSSProperties } from "react"; - import { PencilIcon } from "lucide-react"; +import { + type ComponentPropsWithRef, + type CSSProperties, + useEffect, + useState, +} from "react"; import { HexColorInput, HexColorPicker } from "react-colorful"; import { @@ -10,6 +14,7 @@ import { PopoverContent, PopoverTrigger, } from "@/components"; +import { useDebounceValue, usePrevious } from "@/hooks"; import { cn } from "@/utils"; export function ColorPopover({ @@ -30,23 +35,61 @@ export function ColorPopover({ - - + ); } +/** + * Tracks the pointer locally and commits on a delay, following the same pattern + * as the display-name field in `modules/Styles/VertexStyleRow.tsx`. + * + * `react-colorful` fires `onChange` on every pointermove, and each committed + * style rebuilds every element's data and re-serializes the whole canvas — one + * dropped frame per commit, measured up to ~200ms on a 76 node graph. Local + * state keeps the swatch following the pointer at full rate regardless. + * + * Separate from `ColorPopover` so it mounts with the popover content, which + * Radix unmounts on close: the draft is therefore seeded from `color` on every + * open and cannot drift across openings. + */ +function ColorPicker({ + color, + onColorChange, +}: { + color: string; + onColorChange: (color: string) => void; +}) { + const [draft, setDraft] = useState(color); + const debouncedDraft = useDebounceValue(draft, 150); + const previousDraft = usePrevious(debouncedDraft); + + useEffect(() => { + if (previousDraft === null || previousDraft === debouncedDraft) { + return; + } + onColorChange(debouncedDraft); + }, [debouncedDraft, previousDraft, onColorChange]); + + return ( + <> + + + + ); +} + function ColorSwatch({ color, className, diff --git a/packages/graph-explorer/src/components/Graph/hooks/useManageStyles.ts b/packages/graph-explorer/src/components/Graph/hooks/useManageStyles.ts index d73c43094..28d73e343 100755 --- a/packages/graph-explorer/src/components/Graph/hooks/useManageStyles.ts +++ b/packages/graph-explorer/src/components/Graph/hooks/useManageStyles.ts @@ -125,13 +125,6 @@ export const getStyles = ({ addDefault("edge.connections-filter-out", toCyEdgeStyle(outOfFocusEdgeStyle)); addDefault("edge.out-of-focus", toCyEdgeStyle(outOfFocusEdgeStyle)); - rootStyles.push({ - selector: "node[__iconUrl]", - style: { - "background-image": "data(__iconUrl)", - }, - }); - return rootStyles; }; diff --git a/packages/graph-explorer/src/core/StateProvider/displayVertex.test.ts b/packages/graph-explorer/src/core/StateProvider/displayVertex.test.ts index 7a0373dd6..7fe95a7a9 100644 --- a/packages/graph-explorer/src/core/StateProvider/displayVertex.test.ts +++ b/packages/graph-explorer/src/core/StateProvider/displayVertex.test.ts @@ -14,10 +14,17 @@ import { createVertexId, createVertexType, type DisplayAttribute, + displayVerticesInCanvasSelector, + getAppStore, getRawId, + nodesAtom, + nodesSelectedIdsAtom, schemaAtom, type SchemaStorageModel, + toNodeMap, useDisplayVertexFromVertex, + useDisplayVerticesFromVertices, + useSelectedDisplayVertices, type Vertex, } from "@/core"; import { formatDate, LABELS } from "@/utils"; @@ -238,3 +245,110 @@ describe("useDisplayVertexFromVertex", () => { }; } }); + +describe("displayVerticesInCanvasSelector", () => { + it("should map every node in the canvas, keyed by id, in insertion order", () => { + const store = getAppStore(); + const vertices = [ + createRandomVertex(), + createRandomVertex(), + createRandomVertex(), + ]; + store.set(nodesAtom, toNodeMap(vertices)); + + const result = store.get(displayVerticesInCanvasSelector); + + expect(result.keys().toArray()).toStrictEqual(vertices.map(v => v.id)); + for (const vertex of vertices) { + expect(result.get(vertex.id)?.original).toBe(vertex); + } + }); + + it("should be empty when the canvas is empty", () => { + const store = getAppStore(); + expect(store.get(displayVerticesInCanvasSelector).size).toBe(0); + }); + + it("should reflect nodes added and removed across mutations", () => { + const store = getAppStore(); + const first = createRandomVertex(); + const second = createRandomVertex(); + + store.set(nodesAtom, toNodeMap([first])); + expect( + store.get(displayVerticesInCanvasSelector).keys().toArray(), + ).toStrictEqual([first.id]); + + store.set(nodesAtom, toNodeMap([first, second])); + expect( + store.get(displayVerticesInCanvasSelector).keys().toArray(), + ).toStrictEqual([first.id, second.id]); + + store.set(nodesAtom, toNodeMap([second])); + expect( + store.get(displayVerticesInCanvasSelector).keys().toArray(), + ).toStrictEqual([second.id]); + }); + + /** + * Pins the per-id caching that replaced the array-keyed atom family: an + * unchanged node must not be re-derived when other nodes are added. + */ + it("should keep the same DisplayVertex instance for an unchanged node", () => { + const store = getAppStore(); + const unchanged = createRandomVertex(); + + store.set(nodesAtom, toNodeMap([unchanged])); + const before = store.get(displayVerticesInCanvasSelector).get(unchanged.id); + + store.set(nodesAtom, toNodeMap([unchanged, createRandomVertex()])); + const after = store.get(displayVerticesInCanvasSelector).get(unchanged.id); + + expect(after).toBe(before); + }); +}); + +describe("useSelectedDisplayVertices", () => { + it("should map only the selected nodes", () => { + const selected = createRandomVertex(); + const notSelected = createRandomVertex(); + + const { result } = renderHookWithJotai( + useSelectedDisplayVertices, + store => { + store.set(nodesAtom, toNodeMap([selected, notSelected])); + store.set(nodesSelectedIdsAtom, new Set([selected.id])); + }, + ); + + expect(result.current.map(v => v.id)).toStrictEqual([selected.id]); + }); + + it("should ignore selected ids that are no longer in the canvas", () => { + const missing = createRandomVertex(); + + const { result } = renderHookWithJotai( + useSelectedDisplayVertices, + store => { + store.set(nodesSelectedIdsAtom, new Set([missing.id])); + }, + ); + + expect(result.current).toStrictEqual([]); + }); +}); + +describe("useDisplayVerticesFromVertices", () => { + it("should map arbitrary vertices that are not in the canvas", () => { + const vertices = [createRandomVertex(), createRandomVertex()]; + + const { result } = renderHookWithJotai(() => + useDisplayVerticesFromVertices(vertices), + ); + + expect(result.current.keys().toArray()).toStrictEqual( + vertices.map(v => v.id), + ); + expect(result.current.get(vertices[0].id)?.original).toBe(vertices[0]); + }); +}); diff --git a/packages/graph-explorer/src/core/StateProvider/displayVertex.ts b/packages/graph-explorer/src/core/StateProvider/displayVertex.ts index 5ca4560eb..dc154154c 100644 --- a/packages/graph-explorer/src/core/StateProvider/displayVertex.ts +++ b/packages/graph-explorer/src/core/StateProvider/displayVertex.ts @@ -12,10 +12,11 @@ import { useVertex, type Vertex, type VertexId, - vertexStyleByTypeAtom, + vertexStyleAtom, + type VertexStyleLookup, type VertexType, } from "@/core"; -import { textTransformSelector } from "@/hooks"; +import { type TextTransformer, textTransformSelector } from "@/hooks"; import { LABELS, RESERVED_ID_PROPERTY, RESERVED_TYPES_PROPERTY } from "@/utils"; /** Represents a vertex's display information after all transformations have been applied. */ @@ -46,101 +47,122 @@ export function useDisplayVerticesInCanvas() { /** Maps a `Vertex` instance to a `DisplayVertex` instance using the schema and any user styles. */ export function useDisplayVertexFromVertex(vertex: Vertex) { - return useAtomValue(displayVertexSelector(vertex)); + return toDisplayVertex(vertex, useAtomValue(displayVertexContextSelector)); } /** Maps the `Vertex` instances to a `DisplayVertex` instances using the schema and any user styles. */ export function useDisplayVerticesFromVertices(vertices: Vertex[]) { - return useAtomValue(displayVerticesSelector(vertices)); + const context = useAtomValue(displayVertexContextSelector); + return new Map(vertices.map(v => [v.id, toDisplayVertex(v, context)])); } -const selectedDisplayVerticesSelector = atom(get => { - const selectedIds = get(nodesSelectedIdsAtom); - return selectedIds +const selectedDisplayVerticesSelector = atom(get => + get(nodesSelectedIdsAtom) .values() - .map(id => get(nodeSelector(id))) - .filter(n => n != null) - .map(n => get(displayVertexSelector(n))) - .filter(n => n != null) - .toArray(); -}); + .map(id => get(displayVertexSelector(id))) + .filter(v => v != null) + .toArray(), +); /** Maps all `Vertex` instances which are selected in the graph canvas to `DisplayVertex` instances. */ export function useSelectedDisplayVertices() { return useAtomValue(selectedDisplayVerticesSelector); } -const displayVertexSelector = atomFamily((vertex: Vertex) => +/** + * Everything a `Vertex` needs to become a `DisplayVertex`, resolved once per + * store change instead of once per vertex. + */ +type DisplayVertexContext = { + textTransform: TextTransformer; + isSparql: boolean; + vertexStyles: VertexStyleLookup; +}; + +const displayVertexContextSelector = atom(get => ({ + textTransform: get(textTransformSelector), + isSparql: get(queryEngineSelector) === "sparql", + vertexStyles: get(vertexStyleAtom), +})); + +/** + * Keyed by `VertexId` rather than the `Vertex` object so the family interns one + * entry per node instead of one per object identity, which `nodesAtom` mutations + * would otherwise leak on every recomputation. + */ +const displayVertexSelector = atomFamily((id: VertexId) => atom(get => { - const textTransform = get(textTransformSelector); - const queryEngine = get(queryEngineSelector); - const isSparql = queryEngine === "sparql"; - - const rawStringId = String(getRawId(vertex.id)); - const displayId = isSparql ? textTransform(rawStringId) : rawStringId; - - // List all vertex types for displaying - const vertexTypes = - vertex.types && vertex.types.length > 0 ? vertex.types : [vertex.type]; - const displayTypes = vertexTypes - .map( - type => - get(vertexStyleByTypeAtom(type)).displayLabel ?? textTransform(type), - ) - .join(", "); - - // Map all the attributes for displaying - const sortedAttributes = getSortedDisplayAttributes(vertex, textTransform); - - // Get the display name and description for the vertex - function getDisplayAttributeValueByName(name: string | undefined) { - if (name === RESERVED_ID_PROPERTY) { - return displayId; - } else if (name === RESERVED_TYPES_PROPERTY) { - return displayTypes; - } else if (name) { - return ( - sortedAttributes.find(attr => attr.name === name)?.displayValue ?? - LABELS.MISSING_VALUE - ); - } - - return LABELS.MISSING_VALUE; + const vertex = get(nodeSelector(id)); + if (!vertex) { + return null; } - - const vertexStyle = get(vertexStyleByTypeAtom(vertex.type)); - const displayName = getDisplayAttributeValueByName( - vertexStyle.displayNameAttribute, - ); - const displayDescription = getDisplayAttributeValueByName( - vertexStyle.longDisplayNameAttribute, - ); - - const result: DisplayVertex = { - entityType: "vertex", - id: vertex.id, - primaryType: vertex.type, - types: vertexTypes, - displayId, - displayTypes, - displayName, - displayDescription, - attributes: sortedAttributes, - isBlankNode: vertex.isBlankNode ?? false, - original: vertex, - }; - return result; + return toDisplayVertex(vertex, get(displayVertexContextSelector)); }), ); -const displayVerticesSelector = atomFamily((vertices: Vertex[]) => - atom(get => { - return new Map( - vertices.map(vertex => [vertex.id, get(displayVertexSelector(vertex))]), - ); - }), -); +function toDisplayVertex( + vertex: Vertex, + { textTransform, isSparql, vertexStyles }: DisplayVertexContext, +): DisplayVertex { + const rawStringId = String(getRawId(vertex.id)); + const displayId = isSparql ? textTransform(rawStringId) : rawStringId; + + // List all vertex types for displaying + const vertexTypes = + vertex.types && vertex.types.length > 0 ? vertex.types : [vertex.type]; + const displayTypes = vertexTypes + .map(type => vertexStyles.get(type).displayLabel ?? textTransform(type)) + .join(", "); + + // Map all the attributes for displaying + const sortedAttributes = getSortedDisplayAttributes(vertex, textTransform); + + // Get the display name and description for the vertex + function getDisplayAttributeValueByName(name: string | undefined) { + if (name === RESERVED_ID_PROPERTY) { + return displayId; + } else if (name === RESERVED_TYPES_PROPERTY) { + return displayTypes; + } else if (name) { + return ( + sortedAttributes.find(attr => attr.name === name)?.displayValue ?? + LABELS.MISSING_VALUE + ); + } -const displayVerticesInCanvasSelector = atom(get => { - return get(displayVerticesSelector(get(nodesAtom).values().toArray())); -}); + return LABELS.MISSING_VALUE; + } + + const vertexStyle = vertexStyles.get(vertex.type); + const displayName = getDisplayAttributeValueByName( + vertexStyle.displayNameAttribute, + ); + const displayDescription = getDisplayAttributeValueByName( + vertexStyle.longDisplayNameAttribute, + ); + + return { + entityType: "vertex", + id: vertex.id, + primaryType: vertex.type, + types: vertexTypes, + displayId, + displayTypes, + displayName, + displayDescription, + attributes: sortedAttributes, + isBlankNode: vertex.isBlankNode ?? false, + original: vertex, + }; +} + +export const displayVerticesInCanvasSelector = atom( + get => + new Map( + get(nodesAtom) + .keys() + .map(id => get(displayVertexSelector(id))) + .filter(v => v != null) + .map(v => [v.id, v] as const), + ), +); diff --git a/packages/graph-explorer/src/core/StateProvider/graphElementStyleData.test.ts b/packages/graph-explorer/src/core/StateProvider/graphElementStyleData.test.ts index bbdbdd5da..8e32ff231 100644 --- a/packages/graph-explorer/src/core/StateProvider/graphElementStyleData.test.ts +++ b/packages/graph-explorer/src/core/StateProvider/graphElementStyleData.test.ts @@ -47,9 +47,11 @@ describe("vertexStyleData", () => { ).toBe(1); }); - it("gates __iconUrl on backgroundImage presence", () => { - expect(vertexStyleData(vertex(), undefined).__iconUrl).toBeUndefined(); - expect(vertexStyleData(vertex(), "img").__iconUrl).toBe("img"); + // Always set, never omitted: cytoscape merges element data and never deletes a + // key, so an absent field could not clear a previously applied icon. + it("always sets ge_iconUrl, using none when there is no icon", () => { + expect(vertexStyleData(vertex(), undefined).ge_iconUrl).toBe("none"); + expect(vertexStyleData(vertex(), "img").ge_iconUrl).toBe("img"); }); }); @@ -77,10 +79,10 @@ describe("edgeStyleData", () => { ); }); - it("emits ge_lineDashPattern only for non-solid lines", () => { + it("always sets ge_lineDashPattern, using the default for solid lines", () => { expect( edgeStyleData(edge({ lineStyle: "solid" })).ge_lineDashPattern, - ).toBeUndefined(); + ).toEqual([6, 3]); expect( edgeStyleData(edge({ lineStyle: "dashed" })).ge_lineDashPattern, ).toEqual([5, 6]); @@ -118,14 +120,12 @@ describe("labelTextColorFor", () => { expect(labelTextColorFor("")).toBe("#FFFFFF"); }); - // The result is memoized in a module-level map, so a repeat call must not be - // able to return a different answer than the first. it("returns a stable answer across repeated calls", () => { expect(labelTextColorFor("#123456")).toBe(labelTextColorFor("#123456")); expect(labelTextColorFor("")).toBe(labelTextColorFor("")); }); - it("keys the memo per color rather than sharing one answer", () => { + it("answers per color rather than returning one answer for all", () => { expect(labelTextColorFor("#000000")).not.toBe(labelTextColorFor("#ffffff")); }); }); diff --git a/packages/graph-explorer/src/core/StateProvider/graphElementStyleData.ts b/packages/graph-explorer/src/core/StateProvider/graphElementStyleData.ts index 366e60c1c..1ed1c657d 100644 --- a/packages/graph-explorer/src/core/StateProvider/graphElementStyleData.ts +++ b/packages/graph-explorer/src/core/StateProvider/graphElementStyleData.ts @@ -16,12 +16,32 @@ import { * dash-pattern remap so the style loop stays pure `data()`. */ -/** A `Map` so a type name colliding with `Object.prototype` cannot resolve to a function. */ +/** Cytoscape's own default, used for solid lines, which ignore the pattern. */ +const SOLID_PATTERN: readonly number[] = [6, 3]; + +/** + * A `Map` so a `lineStyle` colliding with `Object.prototype` cannot resolve to a + * function. + */ const LINE_PATTERN = new Map([ + ["solid", SOLID_PATTERN], ["dashed", [5, 6]], ["dotted", [1, 2]], ]); +/** Emitted when a vertex type has no resolved icon; cytoscape's "no image" value. */ +const NO_ICON = "none"; + +/** + * ALWAYS_SET: every field below is set on every element, never omitted. + * + * `cy.json({ elements })` *merges* element data — `ele.data(obj)` adds and + * overwrites keys but never deletes ones missing from the new object. A field + * that is sometimes absent can therefore never be cleared once it has been + * applied, stranding a stale value on an already-drawn element. That is why + * there are no gated `node[…]` / `edge[…]` selectors for these. + */ + /** Data-mapper fields set on every rendered vertex. Feeds the single `node` rule. */ export type VertexStyleData = { ge_color: string; @@ -31,16 +51,16 @@ export type VertexStyleData = { ge_borderOpacity: 0 | 1; ge_borderStyle: LineStyle; ge_shape: VertexStyle["shape"]; - /** Absent when the type has no resolved icon; the `node[__iconUrl]` selector gates on it. */ - __iconUrl?: string; + /** `"none"` when the type has no resolved icon. */ + ge_iconUrl: string; }; /** Data-mapper fields set on every rendered edge. Feeds the single `edge` rule. */ export type EdgeStyleData = { ge_lineColor: string; ge_lineStyle: LineStyle; - /** Absent for solid lines; the `edge[ge_lineDashPattern]` selector gates on it. */ - ge_lineDashPattern?: readonly number[]; + /** Cytoscape's default for solid lines, which ignore it. */ + ge_lineDashPattern: readonly number[]; ge_sourceArrowShape: EdgeStyle["sourceArrowStyle"]; ge_targetArrowShape: EdgeStyle["targetArrowStyle"]; ge_labelTextColor: "#FFFFFF" | "#000000"; @@ -52,27 +72,19 @@ export type EdgeStyleData = { ge_lineThickness: number; }; -/** - * Memoized because parsing a color is the one non-trivial computation in this - * module, and the number of distinct label colors in a graph is tiny next to - * the number of edges asking about them. - */ -const labelTextColors = new Map(); - /** * Picks white-on-dark / black-on-light for a label against its background color. * Falls back to the default label color when unset: an imported style file can * carry an empty `labelColor`, and `new Color("")` throws. + * + * Deliberately not memoized. Profiling this at 0.1ms over a 10s expansion put it + * far below the per-type resolution that dominates, so a module-level cache + * would only add global state shared across stores and tests. */ export function labelTextColorFor(labelColor: string): "#FFFFFF" | "#000000" { - let textColor = labelTextColors.get(labelColor); - if (textColor === undefined) { - textColor = new Color(labelColor || appDefaultEdgeStyle.labelColor).isDark() - ? "#FFFFFF" - : "#000000"; - labelTextColors.set(labelColor, textColor); - } - return textColor; + return new Color(labelColor || appDefaultEdgeStyle.labelColor).isDark() + ? "#FFFFFF" + : "#000000"; } /** Precomputed cytoscape data-mapper fields for a rendered vertex. */ @@ -80,7 +92,7 @@ export function vertexStyleData( style: VertexStyle, backgroundImage: string | undefined, ): VertexStyleData { - const data: VertexStyleData = { + return { ge_color: style.color, ge_backgroundOpacity: style.backgroundOpacity, ge_borderColor: style.borderColor, @@ -88,19 +100,15 @@ export function vertexStyleData( ge_borderOpacity: style.borderWidth > 0 ? 1 : 0, ge_borderStyle: style.borderStyle, ge_shape: style.shape, + ge_iconUrl: backgroundImage ?? NO_ICON, }; - if (backgroundImage !== undefined) { - data.__iconUrl = backgroundImage; - } - return data; } /** Precomputed cytoscape data-mapper fields for a rendered edge. */ export function edgeStyleData(style: EdgeStyle): EdgeStyleData { const lineStyle: LineStyle = style.lineStyle === "dotted" ? "dashed" : style.lineStyle; - const dashPattern = LINE_PATTERN.get(style.lineStyle); - const data: EdgeStyleData = { + return { ge_lineColor: style.lineColor, ge_lineStyle: lineStyle, ge_sourceArrowShape: style.sourceArrowStyle, @@ -112,9 +120,6 @@ export function edgeStyleData(style: EdgeStyle): EdgeStyleData { ge_labelBorderColor: style.labelBorderColor, ge_labelBorderStyle: style.labelBorderStyle, ge_lineThickness: style.lineThickness, + ge_lineDashPattern: LINE_PATTERN.get(style.lineStyle) ?? SOLID_PATTERN, }; - if (dashPattern !== undefined) { - data.ge_lineDashPattern = dashPattern; - } - return data; } diff --git a/packages/graph-explorer/src/core/StateProvider/graphStyles.test.ts b/packages/graph-explorer/src/core/StateProvider/graphStyles.test.ts index 78e4f1b2f..64af85521 100644 --- a/packages/graph-explorer/src/core/StateProvider/graphStyles.test.ts +++ b/packages/graph-explorer/src/core/StateProvider/graphStyles.test.ts @@ -3,6 +3,8 @@ import { useAtomValue } from "jotai"; import { act } from "react"; import { createEdgeType, createVertexType } from "@/core"; +import { RESERVED_ID_PROPERTY, RESERVED_TYPES_PROPERTY } from "@/utils"; +import DEFAULT_ICON_URL from "@/utils/defaultIconUrl"; import { DbState, renderHookWithState } from "@/utils/testing"; import { @@ -10,6 +12,8 @@ import { appDefaultVertexStyle, edgeStyleAtom, type EdgeStyleStorage, + resolveEdgeStyle, + resolveVertexStyle, useEdgeStyling, useVertexStyling, vertexStyleAtom, @@ -36,7 +40,11 @@ function createExpectedEdge(existing: EdgeStyleStorage) { // deliberate and consumers stay trustworthy. describe("app default styles", () => { it("pins the default vertex style values", () => { - expect(appDefaultVertexStyle).toMatchObject({ + expect(appDefaultVertexStyle).toStrictEqual({ + displayNameAttribute: RESERVED_ID_PROPERTY, + longDisplayNameAttribute: RESERVED_TYPES_PROPERTY, + iconUrl: DEFAULT_ICON_URL, + iconImageType: "image/svg+xml", color: "#128EE5", shape: "ellipse", backgroundOpacity: 0.4, @@ -47,9 +55,13 @@ describe("app default styles", () => { }); it("pins the default edge style values", () => { - expect(appDefaultEdgeStyle).toMatchObject({ + expect(appDefaultEdgeStyle).toStrictEqual({ + displayNameAttribute: RESERVED_TYPES_PROPERTY, labelColor: "#17457b", labelBackgroundOpacity: 0.7, + labelBorderColor: "#17457b", + labelBorderStyle: "solid", + labelBorderWidth: 0, lineColor: "#b3b3b3", lineThickness: 2, lineStyle: "solid", @@ -489,3 +501,46 @@ describe("edgeStyleAtom", () => { ); }); }); + +// A style file can carry an optional key explicitly set to undefined. Spreading +// it over the defaults would overwrite them, and the resulting `data()` mapper +// has no missing-value fallback on the cytoscape side. +describe("explicitly undefined optional fields", () => { + it("keeps the edge default rather than taking the undefined", () => { + const type = createEdgeType("knows"); + const resolved = resolveEdgeStyle(type, { + type, + labelBackgroundOpacity: undefined, + lineColor: undefined, + }); + + expect(resolved.labelBackgroundOpacity).toBe( + appDefaultEdgeStyle.labelBackgroundOpacity, + ); + expect(resolved.lineColor).toBe(appDefaultEdgeStyle.lineColor); + }); + + it("keeps the vertex default rather than taking the undefined", () => { + const type = createVertexType("Person"); + const resolved = resolveVertexStyle(type, { + type, + color: undefined, + borderWidth: undefined, + }); + + expect(resolved.color).toBe(appDefaultVertexStyle.color); + expect(resolved.borderWidth).toBe(appDefaultVertexStyle.borderWidth); + }); + + it("still applies a defined override", () => { + const type = createEdgeType("knows"); + const resolved = resolveEdgeStyle(type, { + type, + lineColor: "#abcdef", + labelColor: undefined, + }); + + expect(resolved.lineColor).toBe("#abcdef"); + expect(resolved.labelColor).toBe(appDefaultEdgeStyle.labelColor); + }); +}); diff --git a/packages/graph-explorer/src/core/StateProvider/graphStyles.ts b/packages/graph-explorer/src/core/StateProvider/graphStyles.ts index 8026c7a37..069a25c66 100644 --- a/packages/graph-explorer/src/core/StateProvider/graphStyles.ts +++ b/packages/graph-explorer/src/core/StateProvider/graphStyles.ts @@ -4,12 +4,11 @@ import { atom, useAtomValue, useSetAtom } from "jotai"; import { atomFamily } from "jotai-family"; import { useDeferredValue } from "react"; -import { LABELS, RESERVED_ID_PROPERTY, RESERVED_TYPES_PROPERTY } from "@/utils"; +import { RESERVED_ID_PROPERTY, RESERVED_TYPES_PROPERTY } from "@/utils"; import DEFAULT_ICON_URL from "@/utils/defaultIconUrl"; import type { EdgeType, VertexType } from "../entities"; -import { useActiveSchema } from "./schema"; import { userEdgeStylesAtom, userVertexStylesAtom } from "./storageAtoms"; export const SHAPE_STYLES = [ @@ -199,8 +198,14 @@ export type LegacyUserStylingStorage = { * seam in that parser, not here. */ +/** Resolves the full style of any vertex type. */ +export type VertexStyleLookup = { get(type: VertexType): VertexStyle }; + +/** Resolves the full style of any edge type. */ +export type EdgeStyleLookup = { get(type: EdgeType): EdgeStyle }; + /** Vertex styles indexed by type for O(1) lookup, resolved against defaults. */ -export const vertexStyleAtom = atom(get => { +export const vertexStyleAtom = atom(get => { const userStyles = get(userVertexStylesAtom); return { get(type: VertexType) { @@ -210,7 +215,7 @@ export const vertexStyleAtom = atom(get => { }); /** Edge styles indexed by type for O(1) lookup, resolved against defaults. */ -export const edgeStyleAtom = atom(get => { +export const edgeStyleAtom = atom(get => { const userStyles = get(userEdgeStylesAtom); return { get(type: EdgeType) { @@ -219,6 +224,25 @@ export const edgeStyleAtom = atom(get => { }; }); +/** + * Spreading `user` directly would let a key present but set to `undefined` — an + * imported style file can carry one — overwrite the default with `undefined`, + * which then reaches cytoscape as a `data()` mapper against a missing field and + * cannot fall back. Only keys with a value override. + */ +function withoutUndefined(user: T | undefined): Partial { + if (user === undefined) { + return {}; + } + const defined: Partial = {}; + for (const [key, value] of Object.entries(user)) { + if (value !== undefined) { + defined[key as keyof T] = value as T[keyof T]; + } + } + return defined; +} + /** The user's vertex style overlaid on the app defaults. */ export function resolveVertexStyle( type: VertexType, @@ -227,7 +251,7 @@ export function resolveVertexStyle( return { type, ...appDefaultVertexStyle, - ...user, + ...withoutUndefined(user), } as const; } @@ -239,34 +263,10 @@ export function resolveEdgeStyle( return { type, ...appDefaultEdgeStyle, - ...user, + ...withoutUndefined(user), } as const; } -/** Returns an array of vertex styles based on the known vertex types in the schema. - * Always includes an entry for `LABELS.MISSING_TYPE` so that blank nodes (which are - * assigned that synthetic type at runtime) receive icon styling on the canvas. - */ -export function useAllVertexStyles(): VertexStyle[] { - const styles = useAtomValue(vertexStyleAtom); - const { vertices: allSchemas } = useActiveSchema(); - const schemaStyles = allSchemas.map(({ type }) => styles.get(type)); - - const missingType = LABELS.MISSING_TYPE as VertexType; - const alreadyIncluded = schemaStyles.some(s => s.type === missingType); - if (alreadyIncluded) { - return schemaStyles; - } - return [...schemaStyles, styles.get(missingType)]; -} - -/** Returns an array of edge styles based on the known edge types in the schema. */ -export function useAllEdgeStyles(): EdgeStyle[] { - const styles = useAtomValue(edgeStyleAtom); - const { edges: allSchemas } = useActiveSchema(); - return allSchemas.map(({ type }) => styles.get(type)); -} - /** Returns the resolved style for the specified vertex type. */ export function useVertexStyle(type: VertexType): VertexStyle { return useDeferredValue(useAtomValue(vertexStyleByTypeAtom(type))); diff --git a/packages/graph-explorer/src/core/StateProvider/index.ts b/packages/graph-explorer/src/core/StateProvider/index.ts index 17ba9bb7b..bd5867b3f 100644 --- a/packages/graph-explorer/src/core/StateProvider/index.ts +++ b/packages/graph-explorer/src/core/StateProvider/index.ts @@ -10,8 +10,10 @@ export * from "./featureFlags"; export * from "./neighbors"; export * from "./nodes"; export * from "./renderedEntities"; +export * from "./renderedEntityIds"; export * from "./graphStyles"; export * from "./graphElementStyleData"; +export * from "./styleDataResolvers"; export * from "./schema"; export * from "./storageAtoms"; export * from "./graphSession"; diff --git a/packages/graph-explorer/src/core/StateProvider/renderedEntities.test.ts b/packages/graph-explorer/src/core/StateProvider/renderedEntities.test.ts index 6e9c90e6b..fbccfd7fc 100644 --- a/packages/graph-explorer/src/core/StateProvider/renderedEntities.test.ts +++ b/packages/graph-explorer/src/core/StateProvider/renderedEntities.test.ts @@ -1,11 +1,10 @@ // @vitest-environment happy-dom import { waitFor } from "@testing-library/react"; +import { createStore } from "jotai"; -import { - createEdgeId, - createVertexId, - createVertexType, -} from "@/core/entities"; +import { createVertex, createVertexType } from "@/core/entities"; +import { iconRegistry } from "@/core/icons"; +import { LABELS } from "@/utils"; import { createRandomEdge, createRandomVertex, @@ -14,79 +13,155 @@ import { renderHookWithJotai, } from "@/utils/testing"; +import { canvasVerticesAtom, useRenderedEntities } from "./renderedEntities"; import { createRenderedEdgeId, createRenderedVertexId, - getEdgeIdFromRenderedEdgeId, - getVertexIdFromRenderedVertexId, - type RenderedEdgeId, - type RenderedVertexId, - useRenderedEntities, -} from "./renderedEntities"; - -describe("createRenderedVertexId", () => { - it("should create a rendered vertex id out of a string", () => { - const id = createRenderedVertexId(createVertexId("123")); - expect(id).toBe("(str)123"); - }); +} from "./renderedEntityIds"; - it("should create a rendered vertex id out of a number", () => { - const id = createRenderedVertexId(createVertexId(123)); - expect(id).toBe("(num)123"); +describe("canvasVerticesAtom", () => { + it("should exclude vertices filtered by ID and by type", () => { + const dbState = new DbState(); + const kept = createTestableVertex(); + const filteredById = createTestableVertex(); + const filteredByType = createTestableVertex(); + + dbState.addTestableVertexToGraph(kept); + dbState.addTestableVertexToGraph(filteredById); + dbState.addTestableVertexToGraph(filteredByType); + dbState.filterVertex(filteredById.id); + dbState.filterVertexType(createVertexType(filteredByType.types[0])); + + const store = createStore(); + dbState.applyTo(store); + + const { vertices, ids } = store.get(canvasVerticesAtom); + expect(vertices.map(v => v.vertex.id)).toStrictEqual([kept.id]); + expect([...ids]).toStrictEqual([kept.id]); }); -}); -describe("createRenderedEdgeId", () => { - it("should create a rendered edge id out of a string", () => { - const id = createRenderedEdgeId(createEdgeId("123")); - expect(id).toBe("(str)123"); + // The point of the scoping: a schema can carry far more types than the canvas + // draws, and resolving an icon for every one of them dominated render cost. + it("should cover only the types drawn on the canvas", () => { + const dbState = new DbState(); + const onCanvas = createTestableVertex(); + dbState.addTestableVertexToGraph(onCanvas); + dbState.addVertexStyle(createVertexType("NotOnCanvas"), { + color: "#123456", + }); + + const store = createStore(); + dbState.applyTo(store); + + expect([ + ...store.get(canvasVerticesAtom).stylesByType.keys(), + ]).toStrictEqual([createVertexType(onCanvas.types[0])]); }); - it("should create a rendered edge id out of a number", () => { - const id = createRenderedEdgeId(createEdgeId(123)); - expect(id).toBe("(num)123"); + it("should exclude the types of filtered-out vertices", () => { + const dbState = new DbState(); + const kept = createTestableVertex(); + const filtered = createTestableVertex(); + dbState.addTestableVertexToGraph(kept); + dbState.addTestableVertexToGraph(filtered); + dbState.filterVertex(filtered.id); + + const store = createStore(); + dbState.applyTo(store); + + expect([ + ...store.get(canvasVerticesAtom).stylesByType.keys(), + ]).toStrictEqual([createVertexType(kept.types[0])]); }); -}); -describe("getVertexIdFromRenderedVertexId", () => { - it("should return the raw string id without the prefix", () => { - const id = getVertexIdFromRenderedVertexId( - createRenderedVertexId(createVertexId("123")), + // Blank nodes are assigned the synthetic `LABELS.MISSING_TYPE` by + // `createVertex`, and the canvas scopes styles to the types it draws, so this + // is what keeps a blank node from losing its icon. + it("should cover a blank node's synthetic missing type", () => { + const dbState = new DbState(); + dbState.addVertexToGraph( + createVertex({ id: "blank", isBlankNode: true, types: [] }), ); - expect(id).toBe("123"); + + const store = createStore(); + dbState.applyTo(store); + + expect([ + ...store.get(canvasVerticesAtom).stylesByType.keys(), + ]).toStrictEqual([LABELS.MISSING_TYPE]); }); - it("should return the raw number id without the prefix", () => { - const id = getVertexIdFromRenderedVertexId( - createRenderedVertexId(createVertexId(123)), - ); - expect(id).toBe(123); + it("should list a shared type once", () => { + const dbState = new DbState(); + const first = createTestableVertex(); + const second = createTestableVertex().with({ types: first.types }); + dbState.addTestableVertexToGraph(first); + dbState.addTestableVertexToGraph(second); + + const store = createStore(); + dbState.applyTo(store); + + expect(store.get(canvasVerticesAtom).stylesByType.size).toBe(1); }); - it("should return the id as is if it is not marked as a string or number", () => { - const id = getVertexIdFromRenderedVertexId("123" as RenderedVertexId); - expect(id).toBe("123"); + // The invariant `useRenderedVertices` throws on: every drawn vertex has a + // style, because the same loop produced both. + it("should cover the primary type of every vertex it returns", () => { + const dbState = new DbState(); + const first = createTestableVertex(); + const shared = createTestableVertex().with({ types: first.types }); + const filtered = createTestableVertex(); + dbState.addTestableVertexToGraph(first); + dbState.addTestableVertexToGraph(shared); + dbState.addTestableVertexToGraph(filtered); + dbState.addVertexToGraph( + createVertex({ id: "blank", isBlankNode: true, types: [] }), + ); + dbState.filterVertex(filtered.id); + + const store = createStore(); + dbState.applyTo(store); + + const { vertices, stylesByType } = store.get(canvasVerticesAtom); + expect(vertices).not.toHaveLength(0); + for (const { vertex, style } of vertices) { + // The style travels with the vertex, so this cannot be a miss by design. + expect(style.type).toBe(vertex.primaryType); + expect(stylesByType.get(vertex.primaryType)).toBe(style); + } }); }); -describe("getEdgeIdFromRenderedEdgeId", () => { - it("should return the raw string id without the prefix", () => { - const id = getEdgeIdFromRenderedEdgeId( - createRenderedEdgeId(createEdgeId("123")), - ); - expect(id).toBe("123"); +// The canvas resolves icons only for the types it draws, so the scope and the +// per-element lookup have to agree. A miss is silent — the node just renders +// without an icon — so assert the hit case on a real element. +describe("useRenderedVertices icon coverage", () => { + beforeEach(() => { + iconRegistry.reset(); }); - it("should return the raw number id without the prefix", () => { - const id = getEdgeIdFromRenderedEdgeId( - createRenderedEdgeId(createEdgeId(123)), + it("should carry the resolved icon url onto the element data", async () => { + const dbState = new DbState(); + const vertex = createTestableVertex(); + dbState.addTestableVertexToGraph(vertex); + // A raster icon resolves synchronously in the registry, so no fetch stub. + dbState.addVertexStyle(createVertexType(vertex.types[0]), { + iconUrl: "https://example.test/icon.png", + iconImageType: "image/png", + color: "#abcdef", + }); + + const { result } = renderHookWithJotai( + () => useRenderedEntities(), + store => dbState.applyTo(store), ); - expect(id).toBe(123); - }); - it("should return the id as is if it is not marked as a string or number", () => { - const id = getEdgeIdFromRenderedEdgeId("123" as RenderedEdgeId); - expect(id).toBe("123"); + await waitFor(() => { + expect(result.current.vertices[0].data.ge_iconUrl).toBe( + "https://example.test/icon.png", + ); + expect(result.current.vertices[0].data.ge_color).toBe("#abcdef"); + }); }); }); diff --git a/packages/graph-explorer/src/core/StateProvider/renderedEntities.ts b/packages/graph-explorer/src/core/StateProvider/renderedEntities.ts index 80ccea67a..60e974309 100644 --- a/packages/graph-explorer/src/core/StateProvider/renderedEntities.ts +++ b/packages/graph-explorer/src/core/StateProvider/renderedEntities.ts @@ -1,39 +1,34 @@ -import { useAtomValue } from "jotai"; - -import type { Branded } from "@/utils"; +import { atom, useAtomValue } from "jotai"; import { type DisplayEdge, type DisplayVertex, + displayVerticesInCanvasSelector, + type EdgeType, edgesFilteredIdsAtom, - edgesTypesFilteredAtom, edgeStyleAtom, - type EntityRawId, + edgesTypesFilteredAtom, nodesFilteredIdsAtom, nodesTypesFilteredAtom, useAllNeighbors, - useAllVertexStyles, useDisplayEdgesInCanvas, - useDisplayVerticesInCanvas, - vertexStyleAtom, type VertexId, + type VertexStyle, + vertexStyleAtom, + type VertexType, } from "@/core"; import { useBackgroundImageMap } from "@/core/icons"; -import type { EdgeId } from "../entities/edge"; - import { type EdgeStyleData, edgeStyleData, type VertexStyleData, vertexStyleData, } from "./graphElementStyleData"; - -/** A string representation of a vertex ID that encodes the original type. Cytoscape requires IDs to be strings. */ -export type RenderedVertexId = Branded; - -/** A string representation of an edge ID that encodes the original type. Cytoscape requires IDs to be strings. */ -export type RenderedEdgeId = Branded; +import { + createRenderedEdgeId, + createRenderedVertexId, +} from "./renderedEntityIds"; /** A representation of a vertex that Cytoscape can use. */ export type RenderedVertex = ReturnType; @@ -41,43 +36,83 @@ export type RenderedVertex = ReturnType; /** A representation of an edge that Cytoscape can use. */ export type RenderedEdge = ReturnType; -/** Returns the filtered array of `RenderedVertex` instances for use by Cytoscape. */ -export function useRenderedVertices(): RenderedVertex[] { - const filteredIds = useAtomValue(nodesFilteredIdsAtom); - const filteredTypes = useAtomValue(nodesTypesFilteredAtom); - const displayVerticesInGraph = useDisplayVerticesInCanvas(); - const neighborCounts = useAllNeighbors(); - const vertexStyles = useAtomValue(vertexStyleAtom); - const backgroundImages = useBackgroundImageMap(useAllVertexStyles()); +/** A drawn canvas vertex carrying the resolved style of its primary type. */ +export type CanvasVertex = { + vertex: DisplayVertex; + style: VertexStyle; +}; - const result: RenderedVertex[] = []; +/** + * The canvas vertices that survive filtering, in canvas insertion order, each + * paired with its style, plus their IDs for membership tests and the distinct + * styles of the types drawn. + * + * The style travels with the vertex rather than being looked up later, so a + * drawn vertex cannot fail to have one — there is no lookup to miss and nothing + * to assert. `stylesByType` exists only to scope icon resolution: the schema can + * carry tens of thousands of vertex types while the canvas shows a handful, and + * resolving a style plus an icon for every type in the schema on every render is + * the dominant render cost otherwise. + * + * An atom rather than a hook so both the vertex and edge pipelines share one + * computation per store — as a hook it ran once per call site. + * + * Note this still recomputes when a vertex style changes, because + * `displayVerticesInCanvasSelector` resolves display labels through + * `displayVertexContextSelector`, which reads `vertexStyleAtom`. Decoupling it + * is tracked in #2116. + */ +export const canvasVerticesAtom = atom(get => { + const filteredIds = get(nodesFilteredIdsAtom); + const filteredTypes = get(nodesTypesFilteredAtom); + const displayVertices = get(displayVerticesInCanvasSelector); + const styles = get(vertexStyleAtom); + + const vertices: CanvasVertex[] = []; + const ids = new Set(); + const stylesByType = new Map(); - for (const vertex of displayVerticesInGraph.values()) { + for (const vertex of displayVertices.values()) { // Filters the nodes added to the graph by: // - Individual nodes hidden using the table view // - Vertex types unselected in the filter sidebar if (filteredIds.has(vertex.id)) continue; + if (vertex.types.some(type => filteredTypes.has(type))) continue; + + let style = stylesByType.get(vertex.primaryType); + if (style === undefined) { + style = styles.get(vertex.primaryType); + stylesByType.set(vertex.primaryType, style); + } + + vertices.push({ vertex, style }); + ids.add(vertex.id); + } + + return { vertices, ids, stylesByType }; +}); + +/** Returns the filtered array of `RenderedVertex` instances for use by Cytoscape. */ +export function useRenderedVertices(): RenderedVertex[] { + const { vertices, stylesByType } = useAtomValue(canvasVerticesAtom); + const neighborCounts = useAllNeighbors(); + const backgroundImages = useBackgroundImageMap([...stylesByType.values()]); + + // Resolved on first sight of a type, from the style paired with the vertex, so + // N nodes of a type cost one `vertexStyleData` call and no vertex can be drawn + // without its style. Same shape as `useRenderedEdges` below. + const styleDataByType = new Map(); + const result: RenderedVertex[] = []; - // Check if any vertex type is in the filtered types - let hasFilteredType = false; - for (const type of vertex.types) { - if (filteredTypes.has(type)) { - hasFilteredType = true; - break; - } + for (const { vertex, style } of vertices) { + let styleData = styleDataByType.get(style.type); + if (styleData === undefined) { + styleData = vertexStyleData(style, backgroundImages.get(style.type)); + styleDataByType.set(style.type, styleData); } - if (hasFilteredType) continue; const neighborCount = neighborCounts.get(vertex.id)?.unfetched ?? 0; - const style = vertexStyles.get(vertex.primaryType); - const backgroundImage = backgroundImages.get(vertex.primaryType); - result.push( - createRenderedVertex( - vertex, - neighborCount, - vertexStyleData(style, backgroundImage), - ), - ); + result.push(createRenderedVertex(vertex, neighborCount, styleData)); } return result; @@ -88,12 +123,12 @@ export function useRenderedEdges(): RenderedEdge[] { const edges = useDisplayEdgesInCanvas(); const filteredEdgeIds = useAtomValue(edgesFilteredIdsAtom); const filteredEdgeTypes = useAtomValue(edgesTypesFilteredAtom); - const vertices = useRenderedVertices(); - const edgeStyles = useAtomValue(edgeStyleAtom); - - // Get the IDs of the existing vertices - const existingVertexIds = new Set(vertices.map(v => v.data.vertexId)); + const { ids: visibleVertexIds } = useAtomValue(canvasVerticesAtom); + const styles = useAtomValue(edgeStyleAtom); + // The drawn edge types are only known while filtering, so style data is + // resolved on first sight of a type — one `Color` parse per type, not per edge. + const styleDataByType = new Map(); const result: RenderedEdge[] = []; for (const edge of edges.values()) { @@ -103,11 +138,16 @@ export function useRenderedEdges(): RenderedEdge[] { // - Missing source or target vertex if (filteredEdgeTypes.has(edge.type)) continue; if (filteredEdgeIds.has(edge.id)) continue; - if (!existingVertexIds.has(edge.sourceId)) continue; - if (!existingVertexIds.has(edge.targetId)) continue; + if (!visibleVertexIds.has(edge.sourceId)) continue; + if (!visibleVertexIds.has(edge.targetId)) continue; + + let styleData = styleDataByType.get(edge.type); + if (styleData === undefined) { + styleData = edgeStyleData(styles.get(edge.type)); + styleDataByType.set(edge.type, styleData); + } - const style = edgeStyles.get(edge.type); - result.push(createRenderedEdge(edge, edgeStyleData(style))); + result.push(createRenderedEdge(edge, styleData)); } return result; @@ -119,69 +159,6 @@ export function useRenderedEntities() { return { vertices, edges }; } -/** Maps a VertexId to a string with the original type prefixed. */ -export function createRenderedVertexId(id: VertexId): RenderedVertexId { - return prefixIdWithType(id) as RenderedVertexId; -} - -/** Maps an EdgeId to a string with the original type prefixed. */ -export function createRenderedEdgeId(id: EdgeId): RenderedEdgeId { - return prefixIdWithType(id) as RenderedEdgeId; -} - -/** Strips the ID type prefix from the given ID and returns the value as a VertexId. */ -export function getVertexIdFromRenderedVertexId( - id: RenderedVertexId, -): VertexId { - if (isIdNumber(id)) { - return parseInt(stripIdTypePrefix(id)) as VertexId; - } - if (isIdString(id)) { - return stripIdTypePrefix(id) as VertexId; - } - return String(id) as VertexId; -} - -/** Strips the ID type prefix from the given ID and returns the value as an EdgeId. */ -export function getEdgeIdFromRenderedEdgeId(id: RenderedEdgeId): EdgeId { - if (isIdNumber(id)) { - return parseInt(stripIdTypePrefix(id)) as EdgeId; - } - if (isIdString(id)) { - return stripIdTypePrefix(id) as EdgeId; - } - return String(id) as EdgeId; -} - -const ID_TYPE_NUM_PREFIX = "(num)"; -const ID_TYPE_STR_PREFIX = "(str)"; - -function prefixIdWithType(id: EntityRawId): string { - if (typeof id === "number") { - return `${ID_TYPE_NUM_PREFIX}${id}`; - } - - return `${ID_TYPE_STR_PREFIX}${id}`; -} - -function isIdNumber(id: string): boolean { - return id.startsWith(ID_TYPE_NUM_PREFIX); -} - -function isIdString(id: string): boolean { - return id.startsWith(ID_TYPE_STR_PREFIX); -} - -function stripIdTypePrefix(id: string): string { - if (isIdNumber(id)) { - return id.slice(ID_TYPE_NUM_PREFIX.length); - } - if (isIdString(id)) { - return id.slice(ID_TYPE_STR_PREFIX.length); - } - return id; -} - /** * Creates a representation of a vertex that Cytoscape can use. * diff --git a/packages/graph-explorer/src/core/StateProvider/renderedEntityIds.test.ts b/packages/graph-explorer/src/core/StateProvider/renderedEntityIds.test.ts new file mode 100644 index 000000000..065c2ca09 --- /dev/null +++ b/packages/graph-explorer/src/core/StateProvider/renderedEntityIds.test.ts @@ -0,0 +1,76 @@ +import { createEdgeId, createVertexId } from "@/core/entities"; + +import { + createRenderedEdgeId, + createRenderedVertexId, + getEdgeIdFromRenderedEdgeId, + getVertexIdFromRenderedVertexId, + type RenderedEdgeId, + type RenderedVertexId, +} from "./renderedEntityIds"; + +describe("createRenderedVertexId", () => { + it("should create a rendered vertex id out of a string", () => { + const id = createRenderedVertexId(createVertexId("123")); + expect(id).toBe("(str)123"); + }); + + it("should create a rendered vertex id out of a number", () => { + const id = createRenderedVertexId(createVertexId(123)); + expect(id).toBe("(num)123"); + }); +}); + +describe("createRenderedEdgeId", () => { + it("should create a rendered edge id out of a string", () => { + const id = createRenderedEdgeId(createEdgeId("123")); + expect(id).toBe("(str)123"); + }); + + it("should create a rendered edge id out of a number", () => { + const id = createRenderedEdgeId(createEdgeId(123)); + expect(id).toBe("(num)123"); + }); +}); + +describe("getVertexIdFromRenderedVertexId", () => { + it("should return the raw string id without the prefix", () => { + const id = getVertexIdFromRenderedVertexId( + createRenderedVertexId(createVertexId("123")), + ); + expect(id).toBe("123"); + }); + + it("should return the raw number id without the prefix", () => { + const id = getVertexIdFromRenderedVertexId( + createRenderedVertexId(createVertexId(123)), + ); + expect(id).toBe(123); + }); + + it("should return the id as is if it is not marked as a string or number", () => { + const id = getVertexIdFromRenderedVertexId("123" as RenderedVertexId); + expect(id).toBe("123"); + }); +}); + +describe("getEdgeIdFromRenderedEdgeId", () => { + it("should return the raw string id without the prefix", () => { + const id = getEdgeIdFromRenderedEdgeId( + createRenderedEdgeId(createEdgeId("123")), + ); + expect(id).toBe("123"); + }); + + it("should return the raw number id without the prefix", () => { + const id = getEdgeIdFromRenderedEdgeId( + createRenderedEdgeId(createEdgeId(123)), + ); + expect(id).toBe(123); + }); + + it("should return the id as is if it is not marked as a string or number", () => { + const id = getEdgeIdFromRenderedEdgeId("123" as RenderedEdgeId); + expect(id).toBe("123"); + }); +}); diff --git a/packages/graph-explorer/src/core/StateProvider/renderedEntityIds.ts b/packages/graph-explorer/src/core/StateProvider/renderedEntityIds.ts new file mode 100644 index 000000000..e97097128 --- /dev/null +++ b/packages/graph-explorer/src/core/StateProvider/renderedEntityIds.ts @@ -0,0 +1,80 @@ +/** + * The string encoding Cytoscape requires for element IDs. The prefix records + * whether the original ID was a number or a string so the raw ID can be + * round-tripped back out of the rendered ID. + */ + +import type { Branded } from "@/utils"; + +import type { EdgeId } from "../entities/edge"; +import type { EntityRawId } from "../entities/shared"; +import type { VertexId } from "../entities/vertex"; + +/** A string representation of a vertex ID that encodes the original type. Cytoscape requires IDs to be strings. */ +export type RenderedVertexId = Branded; + +/** A string representation of an edge ID that encodes the original type. Cytoscape requires IDs to be strings. */ +export type RenderedEdgeId = Branded; + +/** Maps a VertexId to a string with the original type prefixed. */ +export function createRenderedVertexId(id: VertexId): RenderedVertexId { + return prefixIdWithType(id) as RenderedVertexId; +} + +/** Maps an EdgeId to a string with the original type prefixed. */ +export function createRenderedEdgeId(id: EdgeId): RenderedEdgeId { + return prefixIdWithType(id) as RenderedEdgeId; +} + +/** Strips the ID type prefix from the given ID and returns the value as a VertexId. */ +export function getVertexIdFromRenderedVertexId( + id: RenderedVertexId, +): VertexId { + if (isIdNumber(id)) { + return parseInt(stripIdTypePrefix(id)) as VertexId; + } + if (isIdString(id)) { + return stripIdTypePrefix(id) as VertexId; + } + return String(id) as VertexId; +} + +/** Strips the ID type prefix from the given ID and returns the value as an EdgeId. */ +export function getEdgeIdFromRenderedEdgeId(id: RenderedEdgeId): EdgeId { + if (isIdNumber(id)) { + return parseInt(stripIdTypePrefix(id)) as EdgeId; + } + if (isIdString(id)) { + return stripIdTypePrefix(id) as EdgeId; + } + return String(id) as EdgeId; +} + +const ID_TYPE_NUM_PREFIX = "(num)"; +const ID_TYPE_STR_PREFIX = "(str)"; + +function prefixIdWithType(id: EntityRawId): string { + if (typeof id === "number") { + return `${ID_TYPE_NUM_PREFIX}${id}`; + } + + return `${ID_TYPE_STR_PREFIX}${id}`; +} + +function isIdNumber(id: string): boolean { + return id.startsWith(ID_TYPE_NUM_PREFIX); +} + +function isIdString(id: string): boolean { + return id.startsWith(ID_TYPE_STR_PREFIX); +} + +function stripIdTypePrefix(id: string): string { + if (isIdNumber(id)) { + return id.slice(ID_TYPE_NUM_PREFIX.length); + } + if (isIdString(id)) { + return id.slice(ID_TYPE_STR_PREFIX.length); + } + return id; +} diff --git a/packages/graph-explorer/src/core/StateProvider/styleDataResolvers.test.ts b/packages/graph-explorer/src/core/StateProvider/styleDataResolvers.test.ts new file mode 100644 index 000000000..5e0fda7ad --- /dev/null +++ b/packages/graph-explorer/src/core/StateProvider/styleDataResolvers.test.ts @@ -0,0 +1,81 @@ +// @vitest-environment happy-dom +import { act, waitFor } from "@testing-library/react"; +import { useAtomValue } from "jotai"; + +import type { AppStore } from "@/core"; + +import { createVertexType, type VertexType } from "@/core/entities"; +import { DbState, renderHookWithJotai } from "@/utils/testing"; + +import { vertexStyleAtom } from "./graphStyles"; +import { userVertexStylesAtom } from "./storageAtoms"; +import { useVertexStyleDataByType } from "./styleDataResolvers"; + +/** Mirrors a caller: resolve the styles for a scope, then build the style data. */ +function useStyleDataForTypes(types: VertexType[]) { + const styles = useAtomValue(vertexStyleAtom); + return useVertexStyleDataByType(types.map(type => styles.get(type))); +} + +describe("useVertexStyleDataByType", () => { + it("should reflect a style edited after the first render", async () => { + const type = createVertexType("Person"); + const dbState = new DbState(); + dbState.addVertexStyle(type, { color: "#111111" }); + + let store!: AppStore; + const { result } = renderHookWithJotai( + () => useStyleDataForTypes([type]), + s => { + store = s; + dbState.applyTo(s); + }, + ); + expect(result.current.get(type)?.ge_color).toBe("#111111"); + + act(() => + store.set(userVertexStylesAtom, prev => + new Map(prev).set(type, { type, color: "#222222" }), + ), + ); + + await waitFor(() => { + expect(result.current.get(type)?.ge_color).toBe("#222222"); + }); + }); + + it("should resolve distinct style data per type", () => { + const dbState = new DbState(); + dbState.addVertexStyle(createVertexType("Person"), { color: "#111111" }); + dbState.addVertexStyle(createVertexType("City"), { color: "#222222" }); + + const { result } = renderHookWithJotai( + () => + useStyleDataForTypes([ + createVertexType("Person"), + createVertexType("City"), + ]), + store => dbState.applyTo(store), + ); + + expect(result.current.get(createVertexType("Person"))?.ge_color).toBe( + "#111111", + ); + expect(result.current.get(createVertexType("City"))?.ge_color).toBe( + "#222222", + ); + }); + + // Scalar fields and icon come from the same list, so a type outside the scope + // is absent rather than present-but-icon-less. + it("should omit a type outside the given styles", () => { + const { result } = renderHookWithJotai(() => + useStyleDataForTypes([createVertexType("Person")]), + ); + + expect(result.current.has(createVertexType("City"))).toBe(false); + expect(result.current.get(createVertexType("Person"))?.ge_iconUrl).toBe( + "none", + ); + }); +}); diff --git a/packages/graph-explorer/src/core/StateProvider/styleDataResolvers.ts b/packages/graph-explorer/src/core/StateProvider/styleDataResolvers.ts new file mode 100644 index 000000000..cfa7c57c9 --- /dev/null +++ b/packages/graph-explorer/src/core/StateProvider/styleDataResolvers.ts @@ -0,0 +1,31 @@ +import { useBackgroundImageMap } from "@/core/icons"; + +import type { VertexType } from "../entities"; +import type { VertexStyle } from "./graphStyles"; + +import { type VertexStyleData, vertexStyleData } from "./graphElementStyleData"; + +/** + * Style data per vertex type. Both the scalar fields and the icon come from the + * same style list, so a type in the result always has both. + * + * Resolving per type rather than per element is what keeps the cost bounded by + * the types drawn: N nodes of a type cost one `vertexStyleData` call, and the + * caller decides the scope — the canvas passes the handful of types it draws, + * the schema view every type. + */ +export function useVertexStyleDataByType( + styles: Iterable, +): Map { + const styleList = [...styles]; + const backgroundImages = useBackgroundImageMap(styleList); + + const result = new Map(); + for (const style of styleList) { + result.set( + style.type, + vertexStyleData(style, backgroundImages.get(style.type)), + ); + } + return result; +} diff --git a/packages/graph-explorer/src/modules/GraphViewer/useGraphStyles.contextCount.test.tsx b/packages/graph-explorer/src/modules/GraphViewer/useGraphStyles.contextCount.test.tsx index 61e03af06..f51b04f89 100644 --- a/packages/graph-explorer/src/modules/GraphViewer/useGraphStyles.contextCount.test.tsx +++ b/packages/graph-explorer/src/modules/GraphViewer/useGraphStyles.contextCount.test.tsx @@ -49,9 +49,13 @@ describe("useGraphStyles style-context count", () => { expect(large).toBe(small); }); - it("emits a small fixed number of selectors, not per-type", async () => { - const count = await selectorCountFor(50); - // node rule + edge rule + at most a couple of gated rules - expect(count).toBeLessThanOrEqual(6); + it("emits exactly the fixed selector set, not per-type", async () => { + const { result } = renderHookWithState( + () => useGraphStyles(), + seedWithTypes(50), + ); + await waitFor(() => expect(result.current).toBeDefined()); + + expect(Object.keys(result.current).sort()).toStrictEqual(["edge", "node"]); }); }); diff --git a/packages/graph-explorer/src/modules/GraphViewer/useGraphStyles.test.tsx b/packages/graph-explorer/src/modules/GraphViewer/useGraphStyles.test.tsx index 242e69e38..dbf05215d 100644 --- a/packages/graph-explorer/src/modules/GraphViewer/useGraphStyles.test.tsx +++ b/packages/graph-explorer/src/modules/GraphViewer/useGraphStyles.test.tsx @@ -1,6 +1,14 @@ // @vitest-environment happy-dom import { describe, expect, it } from "vitest"; +import { + createEdgeType, + createVertexType, + edgeStyleData, + type EdgeStyle, + vertexStyleData, + type VertexStyle, +} from "@/core"; import { renderHookWithState } from "@/utils/testing"; import useGraphStyles from "./useGraphStyles"; @@ -12,7 +20,7 @@ describe("useGraphStyles", () => { it("emits one node rule + one edge rule + one gated dash-pattern rule", () => { const { result } = renderHookWithState(() => useGraphStyles()); const selectors = Object.keys(result.current).sort(); - expect(selectors).toEqual(["edge", "edge[ge_lineDashPattern]", "node"]); + expect(selectors).toEqual(["edge", "node"]); }); it("uses data() mappers for every per-type property", () => { @@ -39,14 +47,98 @@ describe("useGraphStyles", () => { expect(edgeRule["width"]).toBe("data(ge_lineThickness)"); }); - it("gates line-dash-pattern on ge_lineDashPattern presence", () => { - // Solid edges omit the field and fall through to the default; only dashed/ - // dotted edges pick up the pattern via the gated selector. + // No gated selector: every element always sets the field, because cytoscape + // merges element data and could never clear an absent one. + it("maps line-dash-pattern on the base edge rule", () => { const { result } = renderHookWithState(() => useGraphStyles()); - const dashRule = result.current["edge[ge_lineDashPattern]"] as Record< - string, - unknown - >; - expect(dashRule["line-dash-pattern"]).toBe("data(ge_lineDashPattern)"); + const edgeRule = result.current["edge"] as Record; + expect(edgeRule["line-dash-pattern"]).toBe("data(ge_lineDashPattern)"); + }); + + it("maps background-image on the base node rule", () => { + const { result } = renderHookWithState(() => useGraphStyles()); + const nodeRule = result.current["node"] as Record; + expect(nodeRule["background-image"]).toBe("data(ge_iconUrl)"); + }); +}); + +// Producer and consumer must stay in lockstep: a ge_* field added to +// graphElementStyleData without a matching data() mapper (or the reverse) +// silently drops the style. See docs/adr/20260813-element-data-style-mappers.md. +describe("style data round trip", () => { + const vertexStyle: VertexStyle = { + type: createVertexType("Person"), + displayLabel: "Human", + displayNameAttribute: "name", + longDisplayNameAttribute: "bio", + color: "#123456", + iconUrl: "/icons/person.svg", + iconImageType: "image/svg+xml", + shape: "diamond", + backgroundOpacity: 0.42, + borderWidth: 3, + borderColor: "#654321", + borderStyle: "dashed", + }; + + const edgeStyle: EdgeStyle = { + type: createEdgeType("knows"), + displayLabel: "Knows", + displayNameAttribute: "since", + lineColor: "#0a0b0c", + lineThickness: 4, + lineStyle: "dotted", + sourceArrowStyle: "circle", + targetArrowStyle: "tee", + labelColor: "#abcdef", + labelBackgroundOpacity: 0.73, + labelBorderColor: "#fedcba", + labelBorderStyle: "dashed", + labelBorderWidth: 2, + }; + + function producedKeys() { + return new Set([ + ...Object.keys(vertexStyleData(vertexStyle, vertexStyle.iconUrl)), + ...Object.keys(edgeStyleData(edgeStyle)), + ]); + } + + function mappedKeys(styles: ReturnType) { + const keys = new Set(); + for (const rule of Object.values(styles)) { + for (const value of Object.values(rule as Record)) { + for (const [, key] of String(value).matchAll(/data\(([^)]+)\)/g)) { + keys.add(key); + } + } + } + return keys; + } + + it("has a data() mapper for every key the producers emit", () => { + const { result } = renderHookWithState(() => useGraphStyles()); + const mapped = mappedKeys(result.current); + const unmapped = [...producedKeys()].filter(key => !mapped.has(key)).sort(); + + expect( + unmapped, + "style data keys produced with no data() mapper in the stylesheet", + ).toEqual([]); + }); + + it("emits style data for every ge_ mapper in the stylesheet", () => { + const { result } = renderHookWithState(() => useGraphStyles()); + const produced = producedKeys(); + // Non-ge_ mappers (e.g. displayName) come from the rendered entity, not these + // producers, so only ge_ keys are the producers' contract. + const unproduced = [...mappedKeys(result.current)] + .filter(key => key.startsWith("ge_") && !produced.has(key)) + .sort(); + + expect( + unproduced, + "ge_ mappers in the stylesheet that no producer emits", + ).toEqual([]); }); }); diff --git a/packages/graph-explorer/src/modules/GraphViewer/useGraphStyles.ts b/packages/graph-explorer/src/modules/GraphViewer/useGraphStyles.ts index 9aa97d5bb..3d8d9bf6a 100644 --- a/packages/graph-explorer/src/modules/GraphViewer/useGraphStyles.ts +++ b/packages/graph-explorer/src/modules/GraphViewer/useGraphStyles.ts @@ -6,9 +6,12 @@ import type { GraphProps } from "@/components/Graph"; * Every per-type visual value is precomputed onto element `data()` by * `vertexStyleData` / `edgeStyleData` and read back through `data(...)` * mappers, so the stylesheet is O(1) in the number of types rather than one - * selector per type — see #2104. Two gated selectors handle absent-field - * cases (`__iconUrl` for typed icons, `ge_lineDashPattern` for non-solid - * edges). + * selector per type — see #2104. + * + * There are no gated `node[…]` / `edge[…]` selectors: cytoscape merges element + * data and never deletes a key, so a sometimes-absent field would strand a + * stale value on an already-drawn element. Every field is always set, so every + * mapper can live on the base rule. */ export default function useGraphStyles(): NonNullable { return CANVAS_STYLES; @@ -23,6 +26,7 @@ const CANVAS_STYLES: NonNullable = { "border-opacity": "data(ge_borderOpacity)", "border-style": "data(ge_borderStyle)", shape: "data(ge_shape)", + "background-image": "data(ge_iconUrl)", width: 24, height: 24, }, @@ -31,6 +35,7 @@ const CANVAS_STYLES: NonNullable = { color: "data(ge_labelTextColor)", "line-color": "data(ge_lineColor)", "line-style": "data(ge_lineStyle)", + "line-dash-pattern": "data(ge_lineDashPattern)", "source-arrow-shape": "data(ge_sourceArrowShape)", "source-arrow-color": "data(ge_lineColor)", "target-arrow-shape": "data(ge_targetArrowShape)", @@ -46,7 +51,4 @@ const CANVAS_STYLES: NonNullable = { "source-distance-from-node": "0", "target-distance-from-node": "0", }, - "edge[ge_lineDashPattern]": { - "line-dash-pattern": "data(ge_lineDashPattern)", - }, }; diff --git a/packages/graph-explorer/src/modules/SchemaGraph/useSchemaGraphData.test.tsx b/packages/graph-explorer/src/modules/SchemaGraph/useSchemaGraphData.test.tsx index b646276ba..c4575564e 100644 --- a/packages/graph-explorer/src/modules/SchemaGraph/useSchemaGraphData.test.tsx +++ b/packages/graph-explorer/src/modules/SchemaGraph/useSchemaGraphData.test.tsx @@ -1,14 +1,17 @@ // @vitest-environment happy-dom -import { waitFor } from "@testing-library/react"; +import { act, waitFor } from "@testing-library/react"; import { beforeEach, describe, expect, it, type Mock, vi } from "vitest"; -import { createEdgeType, createVertexType } from "@/core"; +import type { AppStore } from "@/core"; + +import { createEdgeType, createVertexType, schemaAtom } from "@/core"; import { useBackgroundImageMap } from "@/core/icons"; import { createRandomEdgeTypeConfig, createRandomVertexTypeConfig, DbState, renderHookWithState, + renderHookWithJotai, } from "@/utils/testing"; import { useSchemaGraphData } from "./useSchemaGraphData"; @@ -48,7 +51,7 @@ describe("useSchemaGraphData", () => { expect(node.data.ge_shape).toBe("hexagon"); expect(node.data.ge_borderWidth).toBe(2); expect(node.data.ge_borderOpacity).toBe(1); - expect(node.data.__iconUrl).toBe("img:Person"); + expect(node.data.ge_iconUrl).toBe("img:Person"); }); it("enriches edges with per-type ge_* style data (solid: no dash pattern)", async () => { @@ -84,10 +87,10 @@ describe("useSchemaGraphData", () => { expect(edge.data.ge_lineColor).toBe("#ff0000"); expect(edge.data.ge_lineStyle).toBe("solid"); expect(edge.data.ge_targetArrowShape).toBe("triangle"); - expect(edge.data.ge_lineDashPattern).toBeUndefined(); + expect(edge.data.ge_lineDashPattern).toEqual([6, 3]); }); - it("emits ge_lineDashPattern for dashed edges", async () => { + it("emits the dashed ge_lineDashPattern for dashed edges", async () => { const vertexConfig = { ...createRandomVertexTypeConfig(), type: createVertexType("Person"), @@ -115,4 +118,51 @@ describe("useSchemaGraphData", () => { expect(result.current.edges[0].data.ge_lineDashPattern).toEqual([5, 6]); }); + + // Regression: the style scope used to be read from `useActiveSchema`, which is + // deferred, while the type configs read the schema atom directly. A schema sync + // that added a label could therefore produce a render with a config whose style + // had not arrived, which threw and took down the whole view. Scope and loop now + // come from the same source, so a newly synced type is always styled. + it("styles a vertex type added by a later schema sync", async () => { + const person = { + ...createRandomVertexTypeConfig(), + type: createVertexType("Person"), + }; + const city = { + ...createRandomVertexTypeConfig(), + type: createVertexType("City"), + color: "#abcdef", + }; + const dbState = new DbState(); + dbState.activeSchema.vertices = [person]; + // Registered up front; the type only enters the schema later. + dbState.addVertexStyle(city.type, { color: "#abcdef" }); + + let store!: AppStore; + const { result } = renderHookWithJotai( + () => useSchemaGraphData(), + s => { + store = s; + dbState.applyTo(s); + }, + ); + await waitFor(() => expect(result.current.nodes.length).toBe(1)); + + act(() => + store.set( + schemaAtom, + new Map([ + [ + dbState.activeConfig.id, + { ...dbState.activeSchema, vertices: [person, city] }, + ], + ]), + ), + ); + + await waitFor(() => expect(result.current.nodes.length).toBe(2)); + const cityNode = result.current.nodes.find(n => n.data.type === city.type); + expect(cityNode?.data.ge_color).toBe("#abcdef"); + }); }); diff --git a/packages/graph-explorer/src/modules/SchemaGraph/useSchemaGraphData.ts b/packages/graph-explorer/src/modules/SchemaGraph/useSchemaGraphData.ts index c69bf8c2e..f89bba589 100644 --- a/packages/graph-explorer/src/modules/SchemaGraph/useSchemaGraphData.ts +++ b/packages/graph-explorer/src/modules/SchemaGraph/useSchemaGraphData.ts @@ -5,20 +5,18 @@ import type { GraphEdge, GraphNode } from "@/components/Graph"; import { createEdgeConnectionId, type EdgeConnectionId, + type EdgeStyleData, edgeStyleAtom, edgeStyleData, - type EdgeStyleData, type EdgeType, useActiveSchema, - useAllVertexStyles, useDisplayEdgeTypeConfigs, useDisplayVertexTypeConfigs, + useVertexStyleDataByType, vertexStyleAtom, - vertexStyleData, type VertexStyleData, type VertexType, } from "@/core"; -import { useBackgroundImageMap } from "@/core/icons"; type SchemaGraphNode = GraphNode & { data: { @@ -51,23 +49,31 @@ export function useSchemaGraphData() { return { nodes, edges }; } -/** Transforms vertex type configs into schema graph nodes. */ +/** + * Transforms vertex type configs into schema graph nodes. + * + * The style scope is derived from the same type configs the loop iterates, so + * the drawn set and the styled set are the same set. Scoping it from the schema + * instead would not be equivalent: `useActiveSchema` is deferred while the type + * configs read the schema atom directly, so mid-sync a render could see a type + * in the configs whose style had not arrived yet. + */ function useSchemaGraphNodes(): SchemaGraphNode[] { const vtConfigs = useDisplayVertexTypeConfigs(); - const vertexStyles = useAtomValue(vertexStyleAtom); - const backgroundImages = useBackgroundImageMap(useAllVertexStyles()); + const styles = useAtomValue(vertexStyleAtom); + const styleDataByType = useVertexStyleDataByType( + vtConfigs.values().map(config => styles.get(config.type)), + ); const nodes: SchemaGraphNode[] = []; - for (const config of vtConfigs.values()) { - const style = vertexStyles.get(config.type); - const backgroundImage = backgroundImages.get(config.type); + for (const [type, styleData] of styleDataByType) { nodes.push({ data: { - id: config.type, - type: config.type, - displayLabel: config.displayLabel, - ...vertexStyleData(style, backgroundImage), + id: type, + type, + displayLabel: vtConfigs.get(type)?.displayLabel ?? type, + ...styleData, }, }); } @@ -82,8 +88,11 @@ function useSchemaGraphEdges( const schema = useActiveSchema(); const edgeConnections = schema.edgeConnections ?? []; const etConfigs = useDisplayEdgeTypeConfigs(); - const edgeStyles = useAtomValue(edgeStyleAtom); + const styles = useAtomValue(edgeStyleAtom); + // Many connections share an edge type, so style data is resolved on first + // sight of a type — one `Color` parse per type, not per connection. + const styleDataByType = new Map(); const edges: SchemaGraphEdge[] = []; for (const connection of edgeConnections) { @@ -93,7 +102,12 @@ function useSchemaGraphEdges( const edgeConfig = etConfigs.get(connection.edgeType); const displayLabel = edgeConfig?.displayLabel ?? connection.edgeType; - const style = edgeStyles.get(connection.edgeType); + + let styleData = styleDataByType.get(connection.edgeType); + if (styleData === undefined) { + styleData = edgeStyleData(styles.get(connection.edgeType)); + styleDataByType.set(connection.edgeType, styleData); + } edges.push({ data: { @@ -102,7 +116,7 @@ function useSchemaGraphEdges( target: connection.targetVertexType, type: connection.edgeType, displayLabel, - ...edgeStyleData(style), + ...styleData, }, }); } diff --git a/packages/graph-explorer/src/modules/SchemaGraph/useSchemaGraphStyles.test.tsx b/packages/graph-explorer/src/modules/SchemaGraph/useSchemaGraphStyles.test.tsx index 4d88b2981..ff0e2aae5 100644 --- a/packages/graph-explorer/src/modules/SchemaGraph/useSchemaGraphStyles.test.tsx +++ b/packages/graph-explorer/src/modules/SchemaGraph/useSchemaGraphStyles.test.tsx @@ -28,7 +28,6 @@ describe("useSchemaGraphStyles", () => { it("stays O(1) in selector count", () => { const { result } = renderHookWithState(() => useSchemaGraphStyles()); - // node + edge + gated edge[ge_lineDashPattern] - expect(Object.keys(result.current!).length).toBeLessThanOrEqual(6); + expect(Object.keys(result.current!).sort()).toStrictEqual(["edge", "node"]); }); });