From 255448521a375fd2690cdcdb345a29fe40348950 Mon Sep 17 00:00:00 2001 From: Kris McGinnes Date: Fri, 14 Aug 2026 14:41:01 -0500 Subject: [PATCH 01/11] Resolve graph style data once per type instead of once per element Style data varies only by type, so within a render pass N elements of a type now cost one `vertexStyleData` call rather than N. The memoizing closure lives in a plain `create*Resolver` factory because the React Compiler lint rules reject a hook that returns a closure mutating its own captured cache. The vertex resolver takes the styles whose icons are in scope, since the canvas needs only the types it draws while the schema view needs every type. Names the atom lookups (`VertexStyleLookup`, `EdgeStyleLookup`) so that contract is explicit, drops the now-callerless `useAllEdgeStyles`, and tightens the style-context test to assert the exact selector set. --- .../src/core/StateProvider/graphStyles.ts | 23 ++-- .../src/core/StateProvider/index.ts | 1 + .../StateProvider/styleDataResolvers.test.ts | 129 ++++++++++++++++++ .../core/StateProvider/styleDataResolvers.ts | 82 +++++++++++ .../useGraphStyles.contextCount.test.tsx | 16 ++- .../modules/SchemaGraph/useSchemaGraphData.ts | 22 +-- 6 files changed, 243 insertions(+), 30 deletions(-) create mode 100644 packages/graph-explorer/src/core/StateProvider/styleDataResolvers.test.ts create mode 100644 packages/graph-explorer/src/core/StateProvider/styleDataResolvers.ts diff --git a/packages/graph-explorer/src/core/StateProvider/graphStyles.ts b/packages/graph-explorer/src/core/StateProvider/graphStyles.ts index 8026c7a37..619767924 100644 --- a/packages/graph-explorer/src/core/StateProvider/graphStyles.ts +++ b/packages/graph-explorer/src/core/StateProvider/graphStyles.ts @@ -199,8 +199,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 +216,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) { @@ -244,8 +250,10 @@ export function resolveEdgeStyle( } /** 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. + * For the schema view, which draws every type; the canvas scopes itself to the + * types it draws via `canvasVertexStylesAtom`. Always includes an entry for + * `LABELS.MISSING_TYPE` so blank nodes (assigned that synthetic type at runtime) + * are styled rather than skipped. */ export function useAllVertexStyles(): VertexStyle[] { const styles = useAtomValue(vertexStyleAtom); @@ -260,13 +268,6 @@ export function useAllVertexStyles(): VertexStyle[] { 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..2f5145f14 100644 --- a/packages/graph-explorer/src/core/StateProvider/index.ts +++ b/packages/graph-explorer/src/core/StateProvider/index.ts @@ -12,6 +12,7 @@ export * from "./nodes"; export * from "./renderedEntities"; 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/styleDataResolvers.test.ts b/packages/graph-explorer/src/core/StateProvider/styleDataResolvers.test.ts new file mode 100644 index 000000000..574fa0b57 --- /dev/null +++ b/packages/graph-explorer/src/core/StateProvider/styleDataResolvers.test.ts @@ -0,0 +1,129 @@ +// @vitest-environment happy-dom +import { act, waitFor } from "@testing-library/react"; + +import type { AppStore } from "@/core"; + +import { createEdgeType, createVertexType } from "@/core/entities"; +import { DbState, renderHookWithJotai } from "@/utils/testing"; + +import { userEdgeStylesAtom, userVertexStylesAtom } from "./storageAtoms"; +import { + useEdgeStyleDataResolver, + useVertexStyleDataResolver, +} from "./styleDataResolvers"; + +describe("useVertexStyleDataResolver", () => { + it("should return the same object for repeated lookups of a type", () => { + const type = createVertexType("Person"); + const { result } = renderHookWithJotai(() => + useVertexStyleDataResolver([]), + ); + + expect(result.current(type)).toBe(result.current(type)); + }); + + // The cache is keyed only by type, so a style edit has to replace the whole + // resolver. If it doesn't, the canvas keeps the old colors. + it("should reflect a style edited after the first lookup", async () => { + const type = createVertexType("Person"); + const dbState = new DbState(); + dbState.addVertexStyle(type, { color: "#111111" }); + + let store!: AppStore; + const { result } = renderHookWithJotai( + () => useVertexStyleDataResolver([]), + s => { + store = s; + dbState.applyTo(s); + }, + ); + expect(result.current(type).ge_color).toBe("#111111"); + + act(() => + store.set(userVertexStylesAtom, prev => + new Map(prev).set(type, { type, color: "#222222" }), + ), + ); + + await waitFor(() => { + expect(result.current(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( + () => useVertexStyleDataResolver([]), + store => dbState.applyTo(store), + ); + + expect(result.current(createVertexType("Person")).ge_color).toBe("#111111"); + expect(result.current(createVertexType("City")).ge_color).toBe("#222222"); + }); + + // The canvas passes only the types it draws, so a type outside that scope must + // still resolve — without an icon. + it("should resolve a type outside the icon scope without an icon", () => { + const { result } = renderHookWithJotai(() => + useVertexStyleDataResolver([]), + ); + + expect( + result.current(createVertexType("Person")).__iconUrl, + ).toBeUndefined(); + }); +}); + +describe("useEdgeStyleDataResolver", () => { + it("should reflect a style edited after the first lookup", async () => { + const type = createEdgeType("route"); + const dbState = new DbState(); + dbState.addEdgeStyle(type, { lineColor: "#111111" }); + + let store!: AppStore; + const { result } = renderHookWithJotai( + () => useEdgeStyleDataResolver(), + s => { + store = s; + dbState.applyTo(s); + }, + ); + expect(result.current(type).ge_lineColor).toBe("#111111"); + + act(() => + store.set(userEdgeStylesAtom, prev => + new Map(prev).set(type, { type, lineColor: "#222222" }), + ), + ); + + await waitFor(() => { + expect(result.current(type).ge_lineColor).toBe("#222222"); + }); + }); + + it("should return the same object for repeated lookups of a type", () => { + const type = createEdgeType("route"); + const { result } = renderHookWithJotai(() => useEdgeStyleDataResolver()); + + expect(result.current(type)).toBe(result.current(type)); + }); + + it("should resolve distinct style data per type", () => { + const dbState = new DbState(); + dbState.addEdgeStyle(createEdgeType("route"), { lineColor: "#111111" }); + dbState.addEdgeStyle(createEdgeType("owns"), { lineColor: "#222222" }); + + const { result } = renderHookWithJotai( + () => useEdgeStyleDataResolver(), + store => dbState.applyTo(store), + ); + + expect(result.current(createEdgeType("route")).ge_lineColor).toBe( + "#111111", + ); + expect(result.current(createEdgeType("owns")).ge_lineColor).toBe("#222222"); + }); +}); 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..4a60444ad --- /dev/null +++ b/packages/graph-explorer/src/core/StateProvider/styleDataResolvers.ts @@ -0,0 +1,82 @@ +import { useAtomValue } from "jotai"; + +import { useBackgroundImageMap } from "@/core/icons"; + +import type { EdgeType, VertexType } from "../entities"; + +import { + type EdgeStyleData, + edgeStyleData, + type VertexStyleData, + vertexStyleData, +} from "./graphElementStyleData"; +import { + type EdgeStyleLookup, + edgeStyleAtom, + type VertexStyle, + type VertexStyleLookup, + vertexStyleAtom, +} from "./graphStyles"; + +/** + * Style data varies only by type, so within one render pass the graph surfaces + * resolve it once per type rather than once per element — N nodes of a type cost + * one `vertexStyleData` call. + * + * The cache is keyed only by type; the styles and icons it was built from are + * pinned by resolver identity alone. It is discarded whenever those inputs + * change identity, which includes every node add or remove, so treat it as a + * per-render dedupe and not a cross-render cache. + */ + +/** Resolves the cytoscape data-mapper fields for a vertex type, memoized per type. */ +export type VertexStyleDataResolver = (type: VertexType) => VertexStyleData; + +/** Resolves the cytoscape data-mapper fields for an edge type, memoized per type. */ +export type EdgeStyleDataResolver = (type: EdgeType) => EdgeStyleData; + +/** + * @param iconStyles The styles whose icons should be resolved — the caller's + * scope, since the canvas needs only the types it draws while the schema view + * needs every type. A type outside this set resolves without an icon. + */ +export function useVertexStyleDataResolver( + iconStyles: VertexStyle[], +): VertexStyleDataResolver { + const styles = useAtomValue(vertexStyleAtom); + const backgroundImages = useBackgroundImageMap(iconStyles); + return createVertexStyleDataResolver(styles, backgroundImages); +} + +export function useEdgeStyleDataResolver(): EdgeStyleDataResolver { + return createEdgeStyleDataResolver(useAtomValue(edgeStyleAtom)); +} + +function createVertexStyleDataResolver( + styles: VertexStyleLookup, + backgroundImages: Map, +): VertexStyleDataResolver { + const cache = new Map(); + return type => { + let data = cache.get(type); + if (data === undefined) { + data = vertexStyleData(styles.get(type), backgroundImages.get(type)); + cache.set(type, data); + } + return data; + }; +} + +function createEdgeStyleDataResolver( + styles: EdgeStyleLookup, +): EdgeStyleDataResolver { + const cache = new Map(); + return type => { + let data = cache.get(type); + if (data === undefined) { + data = edgeStyleData(styles.get(type)); + cache.set(type, data); + } + return data; + }; +} 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..75b4c4f49 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,17 @@ 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", + "edge[ge_lineDashPattern]", + "node", + ]); }); }); diff --git a/packages/graph-explorer/src/modules/SchemaGraph/useSchemaGraphData.ts b/packages/graph-explorer/src/modules/SchemaGraph/useSchemaGraphData.ts index c69bf8c2e..1ce3cd675 100644 --- a/packages/graph-explorer/src/modules/SchemaGraph/useSchemaGraphData.ts +++ b/packages/graph-explorer/src/modules/SchemaGraph/useSchemaGraphData.ts @@ -1,24 +1,19 @@ -import { useAtomValue } from "jotai"; - import type { GraphEdge, GraphNode } from "@/components/Graph"; import { createEdgeConnectionId, type EdgeConnectionId, - edgeStyleAtom, - edgeStyleData, type EdgeStyleData, type EdgeType, useActiveSchema, useAllVertexStyles, useDisplayEdgeTypeConfigs, useDisplayVertexTypeConfigs, - vertexStyleAtom, - vertexStyleData, + useEdgeStyleDataResolver, + useVertexStyleDataResolver, type VertexStyleData, type VertexType, } from "@/core"; -import { useBackgroundImageMap } from "@/core/icons"; type SchemaGraphNode = GraphNode & { data: { @@ -54,20 +49,18 @@ export function useSchemaGraphData() { /** Transforms vertex type configs into schema graph nodes. */ function useSchemaGraphNodes(): SchemaGraphNode[] { const vtConfigs = useDisplayVertexTypeConfigs(); - const vertexStyles = useAtomValue(vertexStyleAtom); - const backgroundImages = useBackgroundImageMap(useAllVertexStyles()); + // The schema view draws every type, so every type's icon is in scope here. + const resolveStyleData = useVertexStyleDataResolver(useAllVertexStyles()); const nodes: SchemaGraphNode[] = []; for (const config of vtConfigs.values()) { - const style = vertexStyles.get(config.type); - const backgroundImage = backgroundImages.get(config.type); nodes.push({ data: { id: config.type, type: config.type, displayLabel: config.displayLabel, - ...vertexStyleData(style, backgroundImage), + ...resolveStyleData(config.type), }, }); } @@ -82,7 +75,7 @@ function useSchemaGraphEdges( const schema = useActiveSchema(); const edgeConnections = schema.edgeConnections ?? []; const etConfigs = useDisplayEdgeTypeConfigs(); - const edgeStyles = useAtomValue(edgeStyleAtom); + const resolveStyleData = useEdgeStyleDataResolver(); const edges: SchemaGraphEdge[] = []; @@ -93,7 +86,6 @@ function useSchemaGraphEdges( const edgeConfig = etConfigs.get(connection.edgeType); const displayLabel = edgeConfig?.displayLabel ?? connection.edgeType; - const style = edgeStyles.get(connection.edgeType); edges.push({ data: { @@ -102,7 +94,7 @@ function useSchemaGraphEdges( target: connection.targetVertexType, type: connection.edgeType, displayLabel, - ...edgeStyleData(style), + ...resolveStyleData(connection.edgeType), }, }); } From 63fa2230b2012fe1c96d04089caa42746cd0a021 Mon Sep 17 00:00:00 2001 From: Kris McGinnes Date: Fri, 14 Aug 2026 14:41:17 -0500 Subject: [PATCH 02/11] Share one visible-vertex computation and scope canvas icons to drawn types MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `useRenderedEdges` called `useRenderedVertices()` while `GraphViewer` also called it directly, so the whole vertex pipeline — icon resolution included — ran twice per render. The filter predicate is now a derived atom both pipelines read, so the store computes it once. Canvas style and icon resolution is scoped to the vertex types actually drawn rather than every type in the schema: the stress schema carries 10,044 vertex types to draw 3, and resolving all of them each render was the dominant app-side render cost. The schema view keeps `useAllVertexStyles`, since drawing every type is its job. Measured over matched 10s windows on a live 10k-type schema, expanding a node: `useBackgroundImageMap` self time 59.7ms -> ~29ms, style work in `renderedEntities` 5.2ms -> ~0.7ms. Total busy time and INP were within run-to-run noise; the dominant expansion cost remains cytoscape rendering and the fcose layout, which this does not touch. --- .../20260813-element-data-style-mappers.md | 1 + docs/agents/react.md | 6 + .../src/core/StateProvider/displayVertex.ts | 2 +- .../StateProvider/renderedEntities.test.ts | 125 ++++++++++++++++++ .../core/StateProvider/renderedEntities.ts | 115 ++++++++++------ 5 files changed, 209 insertions(+), 40 deletions(-) diff --git a/docs/adr/20260813-element-data-style-mappers.md b/docs/adr/20260813-element-data-style-mappers.md index 203efcc67..40a5def7a 100644 --- a/docs/adr/20260813-element-data-style-mappers.md +++ b/docs/adr/20260813-element-data-style-mappers.md @@ -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). Canvas consumers take their scope from `canvasVertexStylesAtom`; only the schema view uses `useAllVertexStyles`, because drawing every type is its actual job. 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/core/StateProvider/displayVertex.ts b/packages/graph-explorer/src/core/StateProvider/displayVertex.ts index 5ca4560eb..03e5eac06 100644 --- a/packages/graph-explorer/src/core/StateProvider/displayVertex.ts +++ b/packages/graph-explorer/src/core/StateProvider/displayVertex.ts @@ -141,6 +141,6 @@ const displayVerticesSelector = atomFamily((vertices: Vertex[]) => }), ); -const displayVerticesInCanvasSelector = atom(get => { +export const displayVerticesInCanvasSelector = atom(get => { return get(displayVerticesSelector(get(nodesAtom).values().toArray())); }); diff --git a/packages/graph-explorer/src/core/StateProvider/renderedEntities.test.ts b/packages/graph-explorer/src/core/StateProvider/renderedEntities.test.ts index 6e9c90e6b..dd66fb031 100644 --- a/packages/graph-explorer/src/core/StateProvider/renderedEntities.test.ts +++ b/packages/graph-explorer/src/core/StateProvider/renderedEntities.test.ts @@ -1,11 +1,15 @@ // @vitest-environment happy-dom import { waitFor } from "@testing-library/react"; +import { createStore } from "jotai"; import { createEdgeId, + createVertex, createVertexId, createVertexType, } from "@/core/entities"; +import { iconRegistry } from "@/core/icons"; +import { LABELS } from "@/utils"; import { createRandomEdge, createRandomVertex, @@ -15,6 +19,7 @@ import { } from "@/utils/testing"; import { + canvasVertexStylesAtom, createRenderedEdgeId, createRenderedVertexId, getEdgeIdFromRenderedEdgeId, @@ -22,6 +27,7 @@ import { type RenderedEdgeId, type RenderedVertexId, useRenderedEntities, + visibleVertexIdsAtom, } from "./renderedEntities"; describe("createRenderedVertexId", () => { @@ -90,6 +96,125 @@ describe("getEdgeIdFromRenderedEdgeId", () => { }); }); +describe("visibleVertexIdsAtom", () => { + 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); + + expect([...store.get(visibleVertexIdsAtom)]).toStrictEqual([kept.id]); + }); +}); + +describe("canvasVertexStylesAtom", () => { + // The point of the atom: 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(canvasVertexStylesAtom).map(s => s.type)).toStrictEqual([ + createVertexType(onCanvas.types[0]), + ]); + }); + + 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(canvasVertexStylesAtom).map(s => s.type)).toStrictEqual([ + createVertexType(kept.types[0]), + ]); + }); + + // `useAllVertexStyles` states this guarantee explicitly; here it has to hold + // via `createVertex` defaulting an untyped vertex to `LABELS.MISSING_TYPE`. + // Without it, blank nodes lose their icon silently. + it("should cover a blank node's synthetic missing type", () => { + const dbState = new DbState(); + dbState.addVertexToGraph( + createVertex({ id: "blank", isBlankNode: true, types: [] }), + ); + + const store = createStore(); + dbState.applyTo(store); + + expect(store.get(canvasVertexStylesAtom).map(s => s.type)).toStrictEqual([ + LABELS.MISSING_TYPE, + ]); + }); + + 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(canvasVertexStylesAtom)).toHaveLength(1); + }); +}); + +// 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 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), + ); + + await waitFor(() => { + expect(result.current.vertices[0].data.__iconUrl).toBe( + "https://example.test/icon.png", + ); + expect(result.current.vertices[0].data.ge_color).toBe("#abcdef"); + }); + }); +}); + describe("useRenderedVertices", () => { it("should return the filtered vertices by ID", async () => { const dbState = new DbState(); diff --git a/packages/graph-explorer/src/core/StateProvider/renderedEntities.ts b/packages/graph-explorer/src/core/StateProvider/renderedEntities.ts index 80ccea67a..d76402a19 100644 --- a/packages/graph-explorer/src/core/StateProvider/renderedEntities.ts +++ b/packages/graph-explorer/src/core/StateProvider/renderedEntities.ts @@ -1,33 +1,32 @@ -import { useAtomValue } from "jotai"; +import { atom, useAtomValue } from "jotai"; import type { Branded } from "@/utils"; import { type DisplayEdge, type DisplayVertex, + displayVerticesInCanvasSelector, edgesFilteredIdsAtom, edgesTypesFilteredAtom, - edgeStyleAtom, type EntityRawId, 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, VertexStyleData } from "./graphElementStyleData"; import { - type EdgeStyleData, - edgeStyleData, - type VertexStyleData, - vertexStyleData, -} from "./graphElementStyleData"; + useEdgeStyleDataResolver, + useVertexStyleDataResolver, +} from "./styleDataResolvers"; /** A string representation of a vertex ID that encodes the original type. Cytoscape requires IDs to be strings. */ export type RenderedVertexId = Branded; @@ -41,41 +40,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()); +/** + * The IDs of the canvas vertices that survive filtering. + * + * 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 + * `vertexStyleByTypeAtom`. Only `id` and `types` are actually needed, so + * sourcing the predicate from `nodesAtom` would decouple it. + */ +export const visibleVertexIdsAtom = atom(get => { + const filteredIds = get(nodesFilteredIdsAtom); + const filteredTypes = get(nodesTypesFilteredAtom); + const displayVerticesInGraph = get(displayVerticesInCanvasSelector); - const result: RenderedVertex[] = []; + const result = new Set(); for (const vertex of displayVerticesInGraph.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; - // 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; - } + result.add(vertex.id); + } + + return result; +}); + +/** + * Vertex styles for only the types drawn on the canvas. + * + * 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. The schema view, which + * genuinely draws every type, uses `useAllVertexStyles` instead. + */ +export const canvasVertexStylesAtom = atom(get => { + const styles = get(vertexStyleAtom); + const displayVerticesInGraph = get(displayVerticesInCanvasSelector); + const visibleIds = get(visibleVertexIdsAtom); + + const types = new Set(); + for (const vertex of displayVerticesInGraph.values()) { + if (visibleIds.has(vertex.id)) { + types.add(vertex.primaryType); } - if (hasFilteredType) continue; + } + + const result: VertexStyle[] = []; + for (const type of types) { + result.push(styles.get(type)); + } + return result; +}); + +/** Returns the filtered array of `RenderedVertex` instances for use by Cytoscape. */ +export function useRenderedVertices(): RenderedVertex[] { + const displayVerticesInGraph = useDisplayVerticesInCanvas(); + const visibleIds = useAtomValue(visibleVertexIdsAtom); + const neighborCounts = useAllNeighbors(); + const canvasVertexStyles = useAtomValue(canvasVertexStylesAtom); + const resolveStyleData = useVertexStyleDataResolver(canvasVertexStyles); + + const result: RenderedVertex[] = []; + + for (const vertex of displayVerticesInGraph.values()) { + if (!visibleIds.has(vertex.id)) 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), + resolveStyleData(vertex.primaryType), ), ); } @@ -88,11 +129,8 @@ 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 visibleVertexIds = useAtomValue(visibleVertexIdsAtom); + const resolveStyleData = useEdgeStyleDataResolver(); const result: RenderedEdge[] = []; @@ -103,11 +141,10 @@ 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; - const style = edgeStyles.get(edge.type); - result.push(createRenderedEdge(edge, edgeStyleData(style))); + result.push(createRenderedEdge(edge, resolveStyleData(edge.type))); } return result; From f7a11cb4949bd01c2396ba1df4952488de4e9f27 Mon Sep 17 00:00:00 2001 From: Kris McGinnes Date: Fri, 14 Aug 2026 14:53:31 -0500 Subject: [PATCH 03/11] Let app defaults win over explicitly undefined style fields An imported style file can carry an optional key present but set to `undefined`. Spreading it over the defaults overwrote them, and the resulting `data()` mapper has no missing-value fallback on the cytoscape side, so the element rendered unstyled for that field. Only keys with a value override now. Also drops the `labelTextColorFor` memo: profiling put it at 0.1ms over a 10s expansion, far below the per-type resolution that dominates, so the cache only bought global state shared across stores and tests. Pins the default style fixtures with `toStrictEqual` so the tests that depend on them are trustworthy. --- .../graphElementStyleData.test.ts | 2 - .../StateProvider/graphElementStyleData.ts | 22 +++---- .../core/StateProvider/graphStyles.test.ts | 59 ++++++++++++++++++- .../src/core/StateProvider/graphStyles.ts | 23 +++++++- 4 files changed, 85 insertions(+), 21 deletions(-) diff --git a/packages/graph-explorer/src/core/StateProvider/graphElementStyleData.test.ts b/packages/graph-explorer/src/core/StateProvider/graphElementStyleData.test.ts index bbdbdd5da..3d05d0049 100644 --- a/packages/graph-explorer/src/core/StateProvider/graphElementStyleData.test.ts +++ b/packages/graph-explorer/src/core/StateProvider/graphElementStyleData.test.ts @@ -118,8 +118,6 @@ 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("")); diff --git a/packages/graph-explorer/src/core/StateProvider/graphElementStyleData.ts b/packages/graph-explorer/src/core/StateProvider/graphElementStyleData.ts index 366e60c1c..a9e2fd440 100644 --- a/packages/graph-explorer/src/core/StateProvider/graphElementStyleData.ts +++ b/packages/graph-explorer/src/core/StateProvider/graphElementStyleData.ts @@ -52,27 +52,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. */ 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 619767924..6c559d5cf 100644 --- a/packages/graph-explorer/src/core/StateProvider/graphStyles.ts +++ b/packages/graph-explorer/src/core/StateProvider/graphStyles.ts @@ -225,6 +225,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, @@ -233,7 +252,7 @@ export function resolveVertexStyle( return { type, ...appDefaultVertexStyle, - ...user, + ...withoutUndefined(user), } as const; } @@ -245,7 +264,7 @@ export function resolveEdgeStyle( return { type, ...appDefaultEdgeStyle, - ...user, + ...withoutUndefined(user), } as const; } From c7fbc9424156efd42546609e2ea8bb7a412205a6 Mon Sep 17 00:00:00 2001 From: Kris McGinnes Date: Fri, 14 Aug 2026 15:00:08 -0500 Subject: [PATCH 04/11] Always set every style data field so a stale value cannot be stranded MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `cy.json({ elements })` merges element data: `ele.data(obj)` adds and overwrites keys but never deletes ones missing from the new object. Both optional fields were therefore unclearable once applied — a node whose icon stopped resolving (an uploaded icon that fails to load, retried three times and given up on) kept rendering the previous icon while its colour updated around it, with no error and no signal to the user that anything had failed. `ge_iconUrl` now carries `"none"` when a type has no icon and `ge_lineDashPattern` carries cytoscape's default for solid lines, so both live on the base `node` / `edge` rule and always reflect current state. That also moves the icon gate out of the generic `components/Graph` component, next to the producer that feeds it, and renames `__iconUrl` to match its siblings. --- .../20260813-element-data-style-mappers.md | 2 +- .../components/Graph/hooks/useManageStyles.ts | 7 --- .../graphElementStyleData.test.ts | 14 +++--- .../StateProvider/graphElementStyleData.ts | 45 ++++++++++++------- .../StateProvider/renderedEntities.test.ts | 2 +- .../StateProvider/styleDataResolvers.test.ts | 4 +- .../useGraphStyles.contextCount.test.tsx | 6 +-- .../GraphViewer/useGraphStyles.test.tsx | 21 +++++---- .../src/modules/GraphViewer/useGraphStyles.ts | 14 +++--- .../SchemaGraph/useSchemaGraphData.test.tsx | 6 +-- .../SchemaGraph/useSchemaGraphStyles.test.tsx | 3 +- 11 files changed, 65 insertions(+), 59 deletions(-) diff --git a/docs/adr/20260813-element-data-style-mappers.md b/docs/adr/20260813-element-data-style-mappers.md index 40a5def7a..4eea05f72 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 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/graphElementStyleData.test.ts b/packages/graph-explorer/src/core/StateProvider/graphElementStyleData.test.ts index 3d05d0049..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]); @@ -123,7 +125,7 @@ describe("labelTextColorFor", () => { 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 a9e2fd440..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"; @@ -72,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, @@ -80,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, @@ -104,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/renderedEntities.test.ts b/packages/graph-explorer/src/core/StateProvider/renderedEntities.test.ts index dd66fb031..9221f32cb 100644 --- a/packages/graph-explorer/src/core/StateProvider/renderedEntities.test.ts +++ b/packages/graph-explorer/src/core/StateProvider/renderedEntities.test.ts @@ -207,7 +207,7 @@ describe("useRenderedVertices icon coverage", () => { ); await waitFor(() => { - expect(result.current.vertices[0].data.__iconUrl).toBe( + 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/styleDataResolvers.test.ts b/packages/graph-explorer/src/core/StateProvider/styleDataResolvers.test.ts index 574fa0b57..64b7476b2 100644 --- a/packages/graph-explorer/src/core/StateProvider/styleDataResolvers.test.ts +++ b/packages/graph-explorer/src/core/StateProvider/styleDataResolvers.test.ts @@ -71,9 +71,7 @@ describe("useVertexStyleDataResolver", () => { useVertexStyleDataResolver([]), ); - expect( - result.current(createVertexType("Person")).__iconUrl, - ).toBeUndefined(); + expect(result.current(createVertexType("Person")).ge_iconUrl).toBe("none"); }); }); 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 75b4c4f49..f51b04f89 100644 --- a/packages/graph-explorer/src/modules/GraphViewer/useGraphStyles.contextCount.test.tsx +++ b/packages/graph-explorer/src/modules/GraphViewer/useGraphStyles.contextCount.test.tsx @@ -56,10 +56,6 @@ describe("useGraphStyles style-context count", () => { ); await waitFor(() => expect(result.current).toBeDefined()); - expect(Object.keys(result.current).sort()).toStrictEqual([ - "edge", - "edge[ge_lineDashPattern]", - "node", - ]); + 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..be0838385 100644 --- a/packages/graph-explorer/src/modules/GraphViewer/useGraphStyles.test.tsx +++ b/packages/graph-explorer/src/modules/GraphViewer/useGraphStyles.test.tsx @@ -12,7 +12,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 +39,17 @@ 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)"); }); }); 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..7cc14844a 100644 --- a/packages/graph-explorer/src/modules/SchemaGraph/useSchemaGraphData.test.tsx +++ b/packages/graph-explorer/src/modules/SchemaGraph/useSchemaGraphData.test.tsx @@ -48,7 +48,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 +84,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"), 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"]); }); }); From 1c85c924194c631100099c09f9355f92c1e0e3f1 Mon Sep 17 00:00:00 2001 From: Kris McGinnes Date: Fri, 14 Aug 2026 15:01:26 -0500 Subject: [PATCH 05/11] Stop interning a dead atom per node-set mutation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `displayVerticesSelector` was an `atomFamily` keyed on a freshly allocated `Vertex[]`, and `displayVertexSelector` on `Vertex` object identity. `atomFamily` caches by parameter identity forever unless `remove`/`setShouldRemove` is called, and nothing here calls either — so every node expansion, add, or remove interned a new entry that could never be reached again, each retaining a `DisplayVertex` for every node on the canvas. Retained memory grew with mutations × nodes for the tab's lifetime, driven by the app's most common interaction. The array-keyed family is gone: `displayVerticesInCanvasSelector` iterates `nodesAtom` through the per-id family in a single pass. `displayVertexSelector` is keyed on `VertexId`, so it holds one entry per node. Vertices that are not on the canvas — search results, and details for a vertex fetched on demand — cannot be served by an id-keyed family, so the derivation is extracted into a pure `toDisplayVertex(vertex, context)` over a non-family context atom. Those callers now intern nothing at all. Retention itself is not asserted: `atomFamily` exposes no size. The added test pins the observable proxy — an unchanged node keeps its `DisplayVertex` identity across an unrelated mutation. --- .../core/StateProvider/displayVertex.test.ts | 114 +++++++++++ .../src/core/StateProvider/displayVertex.ts | 184 ++++++++++-------- 2 files changed, 217 insertions(+), 81 deletions(-) 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 03e5eac06..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 + ); + } -export 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), + ), +); From ead013f0ca0135601615db46bbc47eb38d709190 Mon Sep 17 00:00:00 2001 From: Kris McGinnes Date: Fri, 14 Aug 2026 15:09:34 -0500 Subject: [PATCH 06/11] Resolve canvas vertices, ids, and style scope in one pass MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three passes over the canvas vertices became one. The visible-ids atom iterated every vertex, the style-scope atom iterated them again re-testing membership, and the render hook iterated a third time re-testing it once more — two passes existed only to re-discover a decision the first had already made. `canvasVerticesAtom` returns the drawn vertices, their ids, and the styles of the types they draw from a single loop, so the style scope and the drawn set cannot disagree: a drawn vertex's `primaryType` is in `stylesByType` by construction. That also collapses the reason a missing icon used to be silent. Style data was assembled from two inputs of different totality — scalar fields from a lookup that answers for any type, the icon from a partial scope list — so a scope miss was indistinguishable from a type that genuinely has no icon. The style data now derives entirely from the scoped style list, and a type absent from it is a loud throw rather than a node quietly drawn without its icon. The memoizing resolver layer is gone with it: five exported symbols, two factories and two cache closures replaced by one hook returning a plain per-type map. Edge style data is resolved on first sight of a type inside the existing filter loop, since the drawn edge types are only known while filtering. --- .../src/core/StateProvider/graphStyles.ts | 2 +- .../StateProvider/renderedEntities.test.ts | 60 +++++++--- .../core/StateProvider/renderedEntities.ts | 108 +++++++++-------- .../StateProvider/styleDataResolvers.test.ts | 110 +++++------------- .../core/StateProvider/styleDataResolvers.ts | 97 ++++----------- .../modules/SchemaGraph/useSchemaGraphData.ts | 32 ++++- 6 files changed, 177 insertions(+), 232 deletions(-) diff --git a/packages/graph-explorer/src/core/StateProvider/graphStyles.ts b/packages/graph-explorer/src/core/StateProvider/graphStyles.ts index 6c559d5cf..ac2556c12 100644 --- a/packages/graph-explorer/src/core/StateProvider/graphStyles.ts +++ b/packages/graph-explorer/src/core/StateProvider/graphStyles.ts @@ -270,7 +270,7 @@ export function resolveEdgeStyle( /** Returns an array of vertex styles based on the known vertex types in the schema. * For the schema view, which draws every type; the canvas scopes itself to the - * types it draws via `canvasVertexStylesAtom`. Always includes an entry for + * types it draws via `canvasVerticesAtom`. Always includes an entry for * `LABELS.MISSING_TYPE` so blank nodes (assigned that synthetic type at runtime) * are styled rather than skipped. */ diff --git a/packages/graph-explorer/src/core/StateProvider/renderedEntities.test.ts b/packages/graph-explorer/src/core/StateProvider/renderedEntities.test.ts index 9221f32cb..facb40d62 100644 --- a/packages/graph-explorer/src/core/StateProvider/renderedEntities.test.ts +++ b/packages/graph-explorer/src/core/StateProvider/renderedEntities.test.ts @@ -19,7 +19,7 @@ import { } from "@/utils/testing"; import { - canvasVertexStylesAtom, + canvasVerticesAtom, createRenderedEdgeId, createRenderedVertexId, getEdgeIdFromRenderedEdgeId, @@ -27,7 +27,6 @@ import { type RenderedEdgeId, type RenderedVertexId, useRenderedEntities, - visibleVertexIdsAtom, } from "./renderedEntities"; describe("createRenderedVertexId", () => { @@ -96,7 +95,7 @@ describe("getEdgeIdFromRenderedEdgeId", () => { }); }); -describe("visibleVertexIdsAtom", () => { +describe("canvasVerticesAtom", () => { it("should exclude vertices filtered by ID and by type", () => { const dbState = new DbState(); const kept = createTestableVertex(); @@ -112,12 +111,12 @@ describe("visibleVertexIdsAtom", () => { const store = createStore(); dbState.applyTo(store); - expect([...store.get(visibleVertexIdsAtom)]).toStrictEqual([kept.id]); + const { vertices, ids } = store.get(canvasVerticesAtom); + expect(vertices.map(v => v.id)).toStrictEqual([kept.id]); + expect([...ids]).toStrictEqual([kept.id]); }); -}); -describe("canvasVertexStylesAtom", () => { - // The point of the atom: a schema can carry far more types than the canvas + // 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(); @@ -130,9 +129,9 @@ describe("canvasVertexStylesAtom", () => { const store = createStore(); dbState.applyTo(store); - expect(store.get(canvasVertexStylesAtom).map(s => s.type)).toStrictEqual([ - createVertexType(onCanvas.types[0]), - ]); + expect([ + ...store.get(canvasVerticesAtom).stylesByType.keys(), + ]).toStrictEqual([createVertexType(onCanvas.types[0])]); }); it("should exclude the types of filtered-out vertices", () => { @@ -146,9 +145,9 @@ describe("canvasVertexStylesAtom", () => { const store = createStore(); dbState.applyTo(store); - expect(store.get(canvasVertexStylesAtom).map(s => s.type)).toStrictEqual([ - createVertexType(kept.types[0]), - ]); + expect([ + ...store.get(canvasVerticesAtom).stylesByType.keys(), + ]).toStrictEqual([createVertexType(kept.types[0])]); }); // `useAllVertexStyles` states this guarantee explicitly; here it has to hold @@ -163,9 +162,9 @@ describe("canvasVertexStylesAtom", () => { const store = createStore(); dbState.applyTo(store); - expect(store.get(canvasVertexStylesAtom).map(s => s.type)).toStrictEqual([ - LABELS.MISSING_TYPE, - ]); + expect([ + ...store.get(canvasVerticesAtom).stylesByType.keys(), + ]).toStrictEqual([LABELS.MISSING_TYPE]); }); it("should list a shared type once", () => { @@ -178,7 +177,34 @@ describe("canvasVertexStylesAtom", () => { const store = createStore(); dbState.applyTo(store); - expect(store.get(canvasVertexStylesAtom)).toHaveLength(1); + expect(store.get(canvasVerticesAtom).stylesByType.size).toBe(1); + }); + + // 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 of vertices) { + expect(stylesByType.get(vertex.primaryType)?.type).toBe( + vertex.primaryType, + ); + } }); }); diff --git a/packages/graph-explorer/src/core/StateProvider/renderedEntities.ts b/packages/graph-explorer/src/core/StateProvider/renderedEntities.ts index d76402a19..aefb86d1d 100644 --- a/packages/graph-explorer/src/core/StateProvider/renderedEntities.ts +++ b/packages/graph-explorer/src/core/StateProvider/renderedEntities.ts @@ -6,14 +6,15 @@ import { type DisplayEdge, type DisplayVertex, displayVerticesInCanvasSelector, + type EdgeType, edgesFilteredIdsAtom, + edgeStyleAtom, edgesTypesFilteredAtom, type EntityRawId, nodesFilteredIdsAtom, nodesTypesFilteredAtom, useAllNeighbors, useDisplayEdgesInCanvas, - useDisplayVerticesInCanvas, type VertexId, type VertexStyle, vertexStyleAtom, @@ -21,12 +22,13 @@ import { } from "@/core"; import type { EdgeId } from "../entities/edge"; -import type { EdgeStyleData, VertexStyleData } from "./graphElementStyleData"; import { - useEdgeStyleDataResolver, - useVertexStyleDataResolver, -} from "./styleDataResolvers"; + type EdgeStyleData, + edgeStyleData, + type VertexStyleData, +} from "./graphElementStyleData"; +import { useVertexStyleDataByType } from "./styleDataResolvers"; /** A string representation of a vertex ID that encodes the original type. Cytoscape requires IDs to be strings. */ export type RenderedVertexId = Branded; @@ -41,84 +43,69 @@ export type RenderedVertex = ReturnType; export type RenderedEdge = ReturnType; /** - * The IDs of the canvas vertices that survive filtering. + * The canvas vertices that survive filtering, in canvas insertion order, plus + * their IDs for membership tests and the styles of the types they draw. + * + * One loop, so the style scope and the drawn set cannot disagree: a drawn + * vertex's `primaryType` is in `stylesByType` by construction. 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. The schema view, which genuinely draws + * every type, uses `useAllVertexStyles` instead. * * 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 - * `vertexStyleByTypeAtom`. Only `id` and `types` are actually needed, so - * sourcing the predicate from `nodesAtom` would decouple it. + * `vertexStyleByTypeAtom`. */ -export const visibleVertexIdsAtom = atom(get => { +export const canvasVerticesAtom = atom(get => { const filteredIds = get(nodesFilteredIdsAtom); const filteredTypes = get(nodesTypesFilteredAtom); - const displayVerticesInGraph = get(displayVerticesInCanvasSelector); + const displayVertices = get(displayVerticesInCanvasSelector); + const styles = get(vertexStyleAtom); - const result = new Set(); + const vertices: DisplayVertex[] = []; + 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; - result.add(vertex.id); - } - - return result; -}); - -/** - * Vertex styles for only the types drawn on the canvas. - * - * 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. The schema view, which - * genuinely draws every type, uses `useAllVertexStyles` instead. - */ -export const canvasVertexStylesAtom = atom(get => { - const styles = get(vertexStyleAtom); - const displayVerticesInGraph = get(displayVerticesInCanvasSelector); - const visibleIds = get(visibleVertexIdsAtom); - - const types = new Set(); - for (const vertex of displayVerticesInGraph.values()) { - if (visibleIds.has(vertex.id)) { - types.add(vertex.primaryType); + vertices.push(vertex); + ids.add(vertex.id); + if (!stylesByType.has(vertex.primaryType)) { + stylesByType.set(vertex.primaryType, styles.get(vertex.primaryType)); } } - const result: VertexStyle[] = []; - for (const type of types) { - result.push(styles.get(type)); - } - return result; + return { vertices, ids, stylesByType }; }); /** Returns the filtered array of `RenderedVertex` instances for use by Cytoscape. */ export function useRenderedVertices(): RenderedVertex[] { - const displayVerticesInGraph = useDisplayVerticesInCanvas(); - const visibleIds = useAtomValue(visibleVertexIdsAtom); + const { vertices, stylesByType } = useAtomValue(canvasVerticesAtom); const neighborCounts = useAllNeighbors(); - const canvasVertexStyles = useAtomValue(canvasVertexStylesAtom); - const resolveStyleData = useVertexStyleDataResolver(canvasVertexStyles); + const styleDataByType = useVertexStyleDataByType(stylesByType.values()); const result: RenderedVertex[] = []; - for (const vertex of displayVerticesInGraph.values()) { - if (!visibleIds.has(vertex.id)) continue; + for (const vertex of vertices) { + const styleData = styleDataByType.get(vertex.primaryType); + // `canvasVerticesAtom` scopes the styles to the types it drew. + if (styleData === undefined) { + throw new Error( + `No style data resolved for drawn vertex type "${vertex.primaryType}"`, + ); + } const neighborCount = neighborCounts.get(vertex.id)?.unfetched ?? 0; - result.push( - createRenderedVertex( - vertex, - neighborCount, - resolveStyleData(vertex.primaryType), - ), - ); + result.push(createRenderedVertex(vertex, neighborCount, styleData)); } return result; @@ -129,9 +116,12 @@ export function useRenderedEdges(): RenderedEdge[] { const edges = useDisplayEdgesInCanvas(); const filteredEdgeIds = useAtomValue(edgesFilteredIdsAtom); const filteredEdgeTypes = useAtomValue(edgesTypesFilteredAtom); - const visibleVertexIds = useAtomValue(visibleVertexIdsAtom); - const resolveStyleData = useEdgeStyleDataResolver(); + 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()) { @@ -144,7 +134,13 @@ export function useRenderedEdges(): RenderedEdge[] { if (!visibleVertexIds.has(edge.sourceId)) continue; if (!visibleVertexIds.has(edge.targetId)) continue; - result.push(createRenderedEdge(edge, resolveStyleData(edge.type))); + let styleData = styleDataByType.get(edge.type); + if (styleData === undefined) { + styleData = edgeStyleData(styles.get(edge.type)); + styleDataByType.set(edge.type, styleData); + } + + result.push(createRenderedEdge(edge, styleData)); } return result; diff --git a/packages/graph-explorer/src/core/StateProvider/styleDataResolvers.test.ts b/packages/graph-explorer/src/core/StateProvider/styleDataResolvers.test.ts index 64b7476b2..5e0fda7ad 100644 --- a/packages/graph-explorer/src/core/StateProvider/styleDataResolvers.test.ts +++ b/packages/graph-explorer/src/core/StateProvider/styleDataResolvers.test.ts @@ -1,43 +1,37 @@ // @vitest-environment happy-dom import { act, waitFor } from "@testing-library/react"; +import { useAtomValue } from "jotai"; import type { AppStore } from "@/core"; -import { createEdgeType, createVertexType } from "@/core/entities"; +import { createVertexType, type VertexType } from "@/core/entities"; import { DbState, renderHookWithJotai } from "@/utils/testing"; -import { userEdgeStylesAtom, userVertexStylesAtom } from "./storageAtoms"; -import { - useEdgeStyleDataResolver, - useVertexStyleDataResolver, -} from "./styleDataResolvers"; +import { vertexStyleAtom } from "./graphStyles"; +import { userVertexStylesAtom } from "./storageAtoms"; +import { useVertexStyleDataByType } from "./styleDataResolvers"; -describe("useVertexStyleDataResolver", () => { - it("should return the same object for repeated lookups of a type", () => { - const type = createVertexType("Person"); - const { result } = renderHookWithJotai(() => - useVertexStyleDataResolver([]), - ); - - expect(result.current(type)).toBe(result.current(type)); - }); +/** 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))); +} - // The cache is keyed only by type, so a style edit has to replace the whole - // resolver. If it doesn't, the canvas keeps the old colors. - it("should reflect a style edited after the first lookup", async () => { +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( - () => useVertexStyleDataResolver([]), + () => useStyleDataForTypes([type]), s => { store = s; dbState.applyTo(s); }, ); - expect(result.current(type).ge_color).toBe("#111111"); + expect(result.current.get(type)?.ge_color).toBe("#111111"); act(() => store.set(userVertexStylesAtom, prev => @@ -46,7 +40,7 @@ describe("useVertexStyleDataResolver", () => { ); await waitFor(() => { - expect(result.current(type).ge_color).toBe("#222222"); + expect(result.current.get(type)?.ge_color).toBe("#222222"); }); }); @@ -56,72 +50,32 @@ describe("useVertexStyleDataResolver", () => { dbState.addVertexStyle(createVertexType("City"), { color: "#222222" }); const { result } = renderHookWithJotai( - () => useVertexStyleDataResolver([]), + () => + useStyleDataForTypes([ + createVertexType("Person"), + createVertexType("City"), + ]), store => dbState.applyTo(store), ); - expect(result.current(createVertexType("Person")).ge_color).toBe("#111111"); - expect(result.current(createVertexType("City")).ge_color).toBe("#222222"); - }); - - // The canvas passes only the types it draws, so a type outside that scope must - // still resolve — without an icon. - it("should resolve a type outside the icon scope without an icon", () => { - const { result } = renderHookWithJotai(() => - useVertexStyleDataResolver([]), - ); - - expect(result.current(createVertexType("Person")).ge_iconUrl).toBe("none"); - }); -}); - -describe("useEdgeStyleDataResolver", () => { - it("should reflect a style edited after the first lookup", async () => { - const type = createEdgeType("route"); - const dbState = new DbState(); - dbState.addEdgeStyle(type, { lineColor: "#111111" }); - - let store!: AppStore; - const { result } = renderHookWithJotai( - () => useEdgeStyleDataResolver(), - s => { - store = s; - dbState.applyTo(s); - }, + expect(result.current.get(createVertexType("Person"))?.ge_color).toBe( + "#111111", ); - expect(result.current(type).ge_lineColor).toBe("#111111"); - - act(() => - store.set(userEdgeStylesAtom, prev => - new Map(prev).set(type, { type, lineColor: "#222222" }), - ), + expect(result.current.get(createVertexType("City"))?.ge_color).toBe( + "#222222", ); - - await waitFor(() => { - expect(result.current(type).ge_lineColor).toBe("#222222"); - }); }); - it("should return the same object for repeated lookups of a type", () => { - const type = createEdgeType("route"); - const { result } = renderHookWithJotai(() => useEdgeStyleDataResolver()); - - expect(result.current(type)).toBe(result.current(type)); - }); - - it("should resolve distinct style data per type", () => { - const dbState = new DbState(); - dbState.addEdgeStyle(createEdgeType("route"), { lineColor: "#111111" }); - dbState.addEdgeStyle(createEdgeType("owns"), { lineColor: "#222222" }); - - const { result } = renderHookWithJotai( - () => useEdgeStyleDataResolver(), - store => dbState.applyTo(store), + // 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(createEdgeType("route")).ge_lineColor).toBe( - "#111111", + expect(result.current.has(createVertexType("City"))).toBe(false); + expect(result.current.get(createVertexType("Person"))?.ge_iconUrl).toBe( + "none", ); - expect(result.current(createEdgeType("owns")).ge_lineColor).toBe("#222222"); }); }); diff --git a/packages/graph-explorer/src/core/StateProvider/styleDataResolvers.ts b/packages/graph-explorer/src/core/StateProvider/styleDataResolvers.ts index 4a60444ad..cfa7c57c9 100644 --- a/packages/graph-explorer/src/core/StateProvider/styleDataResolvers.ts +++ b/packages/graph-explorer/src/core/StateProvider/styleDataResolvers.ts @@ -1,82 +1,31 @@ -import { useAtomValue } from "jotai"; - import { useBackgroundImageMap } from "@/core/icons"; -import type { EdgeType, VertexType } from "../entities"; +import type { VertexType } from "../entities"; +import type { VertexStyle } from "./graphStyles"; -import { - type EdgeStyleData, - edgeStyleData, - type VertexStyleData, - vertexStyleData, -} from "./graphElementStyleData"; -import { - type EdgeStyleLookup, - edgeStyleAtom, - type VertexStyle, - type VertexStyleLookup, - vertexStyleAtom, -} from "./graphStyles"; +import { type VertexStyleData, vertexStyleData } from "./graphElementStyleData"; /** - * Style data varies only by type, so within one render pass the graph surfaces - * resolve it once per type rather than once per element — N nodes of a type cost - * one `vertexStyleData` call. + * 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. * - * The cache is keyed only by type; the styles and icons it was built from are - * pinned by resolver identity alone. It is discarded whenever those inputs - * change identity, which includes every node add or remove, so treat it as a - * per-render dedupe and not a cross-render cache. - */ - -/** Resolves the cytoscape data-mapper fields for a vertex type, memoized per type. */ -export type VertexStyleDataResolver = (type: VertexType) => VertexStyleData; - -/** Resolves the cytoscape data-mapper fields for an edge type, memoized per type. */ -export type EdgeStyleDataResolver = (type: EdgeType) => EdgeStyleData; - -/** - * @param iconStyles The styles whose icons should be resolved — the caller's - * scope, since the canvas needs only the types it draws while the schema view - * needs every type. A type outside this set resolves without an icon. + * 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 useVertexStyleDataResolver( - iconStyles: VertexStyle[], -): VertexStyleDataResolver { - const styles = useAtomValue(vertexStyleAtom); - const backgroundImages = useBackgroundImageMap(iconStyles); - return createVertexStyleDataResolver(styles, backgroundImages); -} - -export function useEdgeStyleDataResolver(): EdgeStyleDataResolver { - return createEdgeStyleDataResolver(useAtomValue(edgeStyleAtom)); -} - -function createVertexStyleDataResolver( - styles: VertexStyleLookup, - backgroundImages: Map, -): VertexStyleDataResolver { - const cache = new Map(); - return type => { - let data = cache.get(type); - if (data === undefined) { - data = vertexStyleData(styles.get(type), backgroundImages.get(type)); - cache.set(type, data); - } - return data; - }; -} - -function createEdgeStyleDataResolver( - styles: EdgeStyleLookup, -): EdgeStyleDataResolver { - const cache = new Map(); - return type => { - let data = cache.get(type); - if (data === undefined) { - data = edgeStyleData(styles.get(type)); - cache.set(type, data); - } - return data; - }; +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/SchemaGraph/useSchemaGraphData.ts b/packages/graph-explorer/src/modules/SchemaGraph/useSchemaGraphData.ts index 1ce3cd675..63105143b 100644 --- a/packages/graph-explorer/src/modules/SchemaGraph/useSchemaGraphData.ts +++ b/packages/graph-explorer/src/modules/SchemaGraph/useSchemaGraphData.ts @@ -1,16 +1,19 @@ +import { useAtomValue } from "jotai"; + import type { GraphEdge, GraphNode } from "@/components/Graph"; import { createEdgeConnectionId, type EdgeConnectionId, type EdgeStyleData, + edgeStyleAtom, + edgeStyleData, type EdgeType, useActiveSchema, useAllVertexStyles, useDisplayEdgeTypeConfigs, useDisplayVertexTypeConfigs, - useEdgeStyleDataResolver, - useVertexStyleDataResolver, + useVertexStyleDataByType, type VertexStyleData, type VertexType, } from "@/core"; @@ -50,17 +53,25 @@ export function useSchemaGraphData() { function useSchemaGraphNodes(): SchemaGraphNode[] { const vtConfigs = useDisplayVertexTypeConfigs(); // The schema view draws every type, so every type's icon is in scope here. - const resolveStyleData = useVertexStyleDataResolver(useAllVertexStyles()); + const styleDataByType = useVertexStyleDataByType(useAllVertexStyles()); const nodes: SchemaGraphNode[] = []; for (const config of vtConfigs.values()) { + const styleData = styleDataByType.get(config.type); + // Both the configs and the styles come from the active schema's vertices. + if (styleData === undefined) { + throw new Error( + `No style data resolved for schema vertex type "${config.type}"`, + ); + } + nodes.push({ data: { id: config.type, type: config.type, displayLabel: config.displayLabel, - ...resolveStyleData(config.type), + ...styleData, }, }); } @@ -75,8 +86,11 @@ function useSchemaGraphEdges( const schema = useActiveSchema(); const edgeConnections = schema.edgeConnections ?? []; const etConfigs = useDisplayEdgeTypeConfigs(); - const resolveStyleData = useEdgeStyleDataResolver(); + 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) { @@ -87,6 +101,12 @@ function useSchemaGraphEdges( const edgeConfig = etConfigs.get(connection.edgeType); const displayLabel = edgeConfig?.displayLabel ?? 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: { id: createEdgeConnectionId(connection), @@ -94,7 +114,7 @@ function useSchemaGraphEdges( target: connection.targetVertexType, type: connection.edgeType, displayLabel, - ...resolveStyleData(connection.edgeType), + ...styleData, }, }); } From 777fc86ab8730c6ec2746a6d3e1c0b6590247b58 Mon Sep 17 00:00:00 2001 From: Kris McGinnes Date: Fri, 14 Aug 2026 15:09:51 -0500 Subject: [PATCH 07/11] Enforce the style data producer/consumer lockstep structurally MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The ADR records that a new per-type style property has to be added in two places together — the `ge_*` field and its producer, and the matching `data(…)` mapper — and that editing one side alone silently drops the style. Nothing enforced it, and the label background/border and arrow-colour mappings were asserted nowhere. Asserts both directions by collecting the key sets rather than listing names, so the test cannot rot the way a hand-written list does: every `ge_*` key the producers emit must appear as a mapper somewhere in the stylesheet, and every `ge_*` mapper must have a producer. Non-`ge_*` mappers such as `displayName` come from the rendered entity rather than these producers, so they are excluded by prefix. Walks every rule, so a legitimately re-added gated selector still counts. --- .../GraphViewer/useGraphStyles.test.tsx | 89 +++++++++++++++++++ 1 file changed, 89 insertions(+) diff --git a/packages/graph-explorer/src/modules/GraphViewer/useGraphStyles.test.tsx b/packages/graph-explorer/src/modules/GraphViewer/useGraphStyles.test.tsx index be0838385..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"; @@ -53,3 +61,84 @@ describe("useGraphStyles", () => { 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([]); + }); +}); From 4034084e96b4f70eb0d5caa8cb40d14f72b32c92 Mon Sep 17 00:00:00 2001 From: Kris McGinnes Date: Fri, 14 Aug 2026 15:14:08 -0500 Subject: [PATCH 08/11] Extract the rendered entity ID codec out of renderedEntities MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `renderedEntities.ts` had accumulated four unrelated concerns: a pure string ID codec, the canvas visibility atom, the style scope, and the Cytoscape render hooks. Only the last is what the module name describes. The codec has no coupling to the rest — no React, no Jotai, no state — and is exactly the unit the first four test blocks covered, so it moves out whole along with them. 263 lines down to 193. --- .../src/core/StateProvider/index.ts | 1 + .../StateProvider/renderedEntities.test.ts | 82 +------------------ .../core/StateProvider/renderedEntities.ts | 78 +----------------- .../StateProvider/renderedEntityIds.test.ts | 76 +++++++++++++++++ .../core/StateProvider/renderedEntityIds.ts | 80 ++++++++++++++++++ 5 files changed, 164 insertions(+), 153 deletions(-) create mode 100644 packages/graph-explorer/src/core/StateProvider/renderedEntityIds.test.ts create mode 100644 packages/graph-explorer/src/core/StateProvider/renderedEntityIds.ts diff --git a/packages/graph-explorer/src/core/StateProvider/index.ts b/packages/graph-explorer/src/core/StateProvider/index.ts index 2f5145f14..bd5867b3f 100644 --- a/packages/graph-explorer/src/core/StateProvider/index.ts +++ b/packages/graph-explorer/src/core/StateProvider/index.ts @@ -10,6 +10,7 @@ export * from "./featureFlags"; export * from "./neighbors"; export * from "./nodes"; export * from "./renderedEntities"; +export * from "./renderedEntityIds"; export * from "./graphStyles"; export * from "./graphElementStyleData"; export * from "./styleDataResolvers"; diff --git a/packages/graph-explorer/src/core/StateProvider/renderedEntities.test.ts b/packages/graph-explorer/src/core/StateProvider/renderedEntities.test.ts index facb40d62..e4a95d913 100644 --- a/packages/graph-explorer/src/core/StateProvider/renderedEntities.test.ts +++ b/packages/graph-explorer/src/core/StateProvider/renderedEntities.test.ts @@ -2,12 +2,7 @@ import { waitFor } from "@testing-library/react"; import { createStore } from "jotai"; -import { - createEdgeId, - createVertex, - createVertexId, - createVertexType, -} from "@/core/entities"; +import { createVertex, createVertexType } from "@/core/entities"; import { iconRegistry } from "@/core/icons"; import { LABELS } from "@/utils"; import { @@ -18,82 +13,11 @@ import { renderHookWithJotai, } from "@/utils/testing"; +import { canvasVerticesAtom, useRenderedEntities } from "./renderedEntities"; import { - canvasVerticesAtom, 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"); - }); - - 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"); - }); -}); +} from "./renderedEntityIds"; describe("canvasVerticesAtom", () => { it("should exclude vertices filtered by ID and by type", () => { diff --git a/packages/graph-explorer/src/core/StateProvider/renderedEntities.ts b/packages/graph-explorer/src/core/StateProvider/renderedEntities.ts index aefb86d1d..04a09b542 100644 --- a/packages/graph-explorer/src/core/StateProvider/renderedEntities.ts +++ b/packages/graph-explorer/src/core/StateProvider/renderedEntities.ts @@ -1,7 +1,5 @@ import { atom, useAtomValue } from "jotai"; -import type { Branded } from "@/utils"; - import { type DisplayEdge, type DisplayVertex, @@ -10,7 +8,6 @@ import { edgesFilteredIdsAtom, edgeStyleAtom, edgesTypesFilteredAtom, - type EntityRawId, nodesFilteredIdsAtom, nodesTypesFilteredAtom, useAllNeighbors, @@ -21,21 +18,17 @@ import { type VertexType, } from "@/core"; -import type { EdgeId } from "../entities/edge"; - import { type EdgeStyleData, edgeStyleData, type VertexStyleData, } from "./graphElementStyleData"; +import { + createRenderedEdgeId, + createRenderedVertexId, +} from "./renderedEntityIds"; import { useVertexStyleDataByType } from "./styleDataResolvers"; -/** 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; - /** A representation of a vertex that Cytoscape can use. */ export type RenderedVertex = ReturnType; @@ -152,69 +145,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; +} From 39d46447f46580460dbd0b5523d761f523b1705c Mon Sep 17 00:00:00 2001 From: Kris McGinnes Date: Fri, 14 Aug 2026 15:27:23 -0500 Subject: [PATCH 09/11] Commit color picker changes on a delay instead of per pointermove MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `react-colorful` fires `onChange` on every pointermove and the picker was wired straight to the style atom, so a color drag committed at pointer rate. Each commit rebuilds every rendered element's data and re-serializes the whole canvas through `cy.json` — measured at one dropped frame per commit, 167-217ms on a 76 node graph, so a drag cannot keep up. The picker now tracks the pointer in local state and commits on a 150ms delay, the same shape as the display-name field in `modules/Styles/VertexStyleRow.tsx`. The swatch still follows the pointer at full rate; only the canvas lags. The per-commit cost above is measured. The improvement is not: driving rapid trusted input at the Radix slider is not reachable from the automation harness — synthetic key events are ignored, and real ones arrive too far apart to exercise the debounce — so the reduction in commit count is by construction rather than observed. --- .../src/components/ColorPopover.tsx | 71 +++++++++++++++---- 1 file changed, 57 insertions(+), 14 deletions(-) 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, From d6bfe2588134a832592dee7c78e3f07c118a9764 Mon Sep 17 00:00:00 2001 From: Kris McGinnes Date: Fri, 14 Aug 2026 16:07:28 -0500 Subject: [PATCH 10/11] Name the real style dependency path on the canvas vertices atom The comment cited `vertexStyleByTypeAtom`, which the atom does not reach. The actual path is `displayVerticesInCanvasSelector` -> `displayVertexContextSelector` -> `vertexStyleAtom`, and it is why a style edit still recomputes the canvas pipeline. Points at the follow-up issue rather than leaving a dead end. --- .../graph-explorer/src/core/StateProvider/renderedEntities.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/packages/graph-explorer/src/core/StateProvider/renderedEntities.ts b/packages/graph-explorer/src/core/StateProvider/renderedEntities.ts index 04a09b542..fe322541e 100644 --- a/packages/graph-explorer/src/core/StateProvider/renderedEntities.ts +++ b/packages/graph-explorer/src/core/StateProvider/renderedEntities.ts @@ -51,7 +51,8 @@ export type RenderedEdge = ReturnType; * * Note this still recomputes when a vertex style changes, because * `displayVerticesInCanvasSelector` resolves display labels through - * `vertexStyleByTypeAtom`. + * `displayVertexContextSelector`, which reads `vertexStyleAtom`. Decoupling it + * is tracked in #2116. */ export const canvasVerticesAtom = atom(get => { const filteredIds = get(nodesFilteredIdsAtom); From f30b00592262260fdae38e4db8d5dfc1bb0e656f Mon Sep 17 00:00:00 2001 From: Kris McGinnes Date: Fri, 14 Aug 2026 17:18:20 -0500 Subject: [PATCH 11/11] Make a styleless element unrepresentable instead of fatal MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both style-data lookups guarded coverage with a throw. In render, that unwinds to the app-level boundary in `DefaultLayout` — neither graph view has a local one — so the whole UI is replaced by the error page whose only affordance is a reload. That is a wildly disproportionate outcome for an invariant whose worst alternative is a node drawn without its icon. The schema view's throw was reachable. Its two sides read the same schema by different paths: the type configs read the schema atom directly, while the style scope came from `useAllVertexStyles` -> `useActiveSchema`, which is deferred. A sync that added a vertex label could therefore render a config whose style had not arrived and take down the view. Verified by reproducing the old shape against the new test, which throws on the added type. Note the pre-branch code read `vertexStyleAtom` directly, which is total, so an unknown type simply got default styling — this branch had converted a self-healing case into a crash. Both are now structural rather than asserted. `canvasVerticesAtom` pairs each drawn vertex with its style so there is no lookup to miss, and style data is resolved on first sight of a type from the style in hand — the same shape `useRenderedEdges` already used. The schema view scopes styles from the same type configs its loop iterates. Both throws are gone. `useAllVertexStyles` has no callers left and is deleted; the canvas covers blank nodes through the synthetic type on the vertex itself, which is already tested. --- .../20260813-element-data-style-mappers.md | 2 +- .../src/core/StateProvider/graphStyles.ts | 22 +------- .../StateProvider/renderedEntities.test.ts | 16 +++--- .../core/StateProvider/renderedEntities.ts | 51 +++++++++++------- .../SchemaGraph/useSchemaGraphData.test.tsx | 54 ++++++++++++++++++- .../modules/SchemaGraph/useSchemaGraphData.ts | 34 ++++++------ 6 files changed, 112 insertions(+), 67 deletions(-) diff --git a/docs/adr/20260813-element-data-style-mappers.md b/docs/adr/20260813-element-data-style-mappers.md index 4eea05f72..f4eb4884e 100644 --- a/docs/adr/20260813-element-data-style-mappers.md +++ b/docs/adr/20260813-element-data-style-mappers.md @@ -21,4 +21,4 @@ Precompute each element's resolved style values onto its Cytoscape `ele.data()` - **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). Canvas consumers take their scope from `canvasVertexStylesAtom`; only the schema view uses `useAllVertexStyles`, because drawing every type is its actual job. Edges have no canvas-scoped equivalent on purpose — edge style data needs no icon resolution, so scoping would buy nothing. +- **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/packages/graph-explorer/src/core/StateProvider/graphStyles.ts b/packages/graph-explorer/src/core/StateProvider/graphStyles.ts index ac2556c12..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 = [ @@ -268,25 +267,6 @@ export function resolveEdgeStyle( } as const; } -/** Returns an array of vertex styles based on the known vertex types in the schema. - * For the schema view, which draws every type; the canvas scopes itself to the - * types it draws via `canvasVerticesAtom`. Always includes an entry for - * `LABELS.MISSING_TYPE` so blank nodes (assigned that synthetic type at runtime) - * are styled rather than skipped. - */ -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 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/renderedEntities.test.ts b/packages/graph-explorer/src/core/StateProvider/renderedEntities.test.ts index e4a95d913..fbccfd7fc 100644 --- a/packages/graph-explorer/src/core/StateProvider/renderedEntities.test.ts +++ b/packages/graph-explorer/src/core/StateProvider/renderedEntities.test.ts @@ -36,7 +36,7 @@ describe("canvasVerticesAtom", () => { dbState.applyTo(store); const { vertices, ids } = store.get(canvasVerticesAtom); - expect(vertices.map(v => v.id)).toStrictEqual([kept.id]); + expect(vertices.map(v => v.vertex.id)).toStrictEqual([kept.id]); expect([...ids]).toStrictEqual([kept.id]); }); @@ -74,9 +74,9 @@ describe("canvasVerticesAtom", () => { ]).toStrictEqual([createVertexType(kept.types[0])]); }); - // `useAllVertexStyles` states this guarantee explicitly; here it has to hold - // via `createVertex` defaulting an untyped vertex to `LABELS.MISSING_TYPE`. - // Without it, blank nodes lose their icon silently. + // 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( @@ -124,10 +124,10 @@ describe("canvasVerticesAtom", () => { const { vertices, stylesByType } = store.get(canvasVerticesAtom); expect(vertices).not.toHaveLength(0); - for (const vertex of vertices) { - expect(stylesByType.get(vertex.primaryType)?.type).toBe( - vertex.primaryType, - ); + 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); } }); }); diff --git a/packages/graph-explorer/src/core/StateProvider/renderedEntities.ts b/packages/graph-explorer/src/core/StateProvider/renderedEntities.ts index fe322541e..60e974309 100644 --- a/packages/graph-explorer/src/core/StateProvider/renderedEntities.ts +++ b/packages/graph-explorer/src/core/StateProvider/renderedEntities.ts @@ -17,17 +17,18 @@ import { vertexStyleAtom, type VertexType, } from "@/core"; +import { useBackgroundImageMap } from "@/core/icons"; import { type EdgeStyleData, edgeStyleData, type VertexStyleData, + vertexStyleData, } from "./graphElementStyleData"; import { createRenderedEdgeId, createRenderedVertexId, } from "./renderedEntityIds"; -import { useVertexStyleDataByType } from "./styleDataResolvers"; /** A representation of a vertex that Cytoscape can use. */ export type RenderedVertex = ReturnType; @@ -35,16 +36,23 @@ export type RenderedVertex = ReturnType; /** A representation of an edge that Cytoscape can use. */ export type RenderedEdge = ReturnType; +/** A drawn canvas vertex carrying the resolved style of its primary type. */ +export type CanvasVertex = { + vertex: DisplayVertex; + style: VertexStyle; +}; + /** - * The canvas vertices that survive filtering, in canvas insertion order, plus - * their IDs for membership tests and the styles of the types they draw. + * 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. * - * One loop, so the style scope and the drawn set cannot disagree: a drawn - * vertex's `primaryType` is in `stylesByType` by construction. The schema can + * 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. The schema view, which genuinely draws - * every type, uses `useAllVertexStyles` instead. + * 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. @@ -60,7 +68,7 @@ export const canvasVerticesAtom = atom(get => { const displayVertices = get(displayVerticesInCanvasSelector); const styles = get(vertexStyleAtom); - const vertices: DisplayVertex[] = []; + const vertices: CanvasVertex[] = []; const ids = new Set(); const stylesByType = new Map(); @@ -71,11 +79,14 @@ export const canvasVerticesAtom = atom(get => { if (filteredIds.has(vertex.id)) continue; if (vertex.types.some(type => filteredTypes.has(type))) continue; - vertices.push(vertex); - ids.add(vertex.id); - if (!stylesByType.has(vertex.primaryType)) { - stylesByType.set(vertex.primaryType, styles.get(vertex.primaryType)); + 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 }; @@ -85,17 +96,19 @@ export const canvasVerticesAtom = atom(get => { export function useRenderedVertices(): RenderedVertex[] { const { vertices, stylesByType } = useAtomValue(canvasVerticesAtom); const neighborCounts = useAllNeighbors(); - const styleDataByType = useVertexStyleDataByType(stylesByType.values()); + 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[] = []; - for (const vertex of vertices) { - const styleData = styleDataByType.get(vertex.primaryType); - // `canvasVerticesAtom` scopes the styles to the types it drew. + for (const { vertex, style } of vertices) { + let styleData = styleDataByType.get(style.type); if (styleData === undefined) { - throw new Error( - `No style data resolved for drawn vertex type "${vertex.primaryType}"`, - ); + styleData = vertexStyleData(style, backgroundImages.get(style.type)); + styleDataByType.set(style.type, styleData); } const neighborCount = neighborCounts.get(vertex.id)?.unfetched ?? 0; diff --git a/packages/graph-explorer/src/modules/SchemaGraph/useSchemaGraphData.test.tsx b/packages/graph-explorer/src/modules/SchemaGraph/useSchemaGraphData.test.tsx index 7cc14844a..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"; @@ -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 63105143b..f89bba589 100644 --- a/packages/graph-explorer/src/modules/SchemaGraph/useSchemaGraphData.ts +++ b/packages/graph-explorer/src/modules/SchemaGraph/useSchemaGraphData.ts @@ -10,10 +10,10 @@ import { edgeStyleData, type EdgeType, useActiveSchema, - useAllVertexStyles, useDisplayEdgeTypeConfigs, useDisplayVertexTypeConfigs, useVertexStyleDataByType, + vertexStyleAtom, type VertexStyleData, type VertexType, } from "@/core"; @@ -49,28 +49,30 @@ 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(); - // The schema view draws every type, so every type's icon is in scope here. - const styleDataByType = useVertexStyleDataByType(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 styleData = styleDataByType.get(config.type); - // Both the configs and the styles come from the active schema's vertices. - if (styleData === undefined) { - throw new Error( - `No style data resolved for schema vertex type "${config.type}"`, - ); - } - + for (const [type, styleData] of styleDataByType) { nodes.push({ data: { - id: config.type, - type: config.type, - displayLabel: config.displayLabel, + id: type, + type, + displayLabel: vtConfigs.get(type)?.displayLabel ?? type, ...styleData, }, });