From 4cd7a4e8a7461799e9be2d36873b7207c53d9fd2 Mon Sep 17 00:00:00 2001 From: Kris McGinnes Date: Fri, 14 Aug 2026 15:00:08 -0500 Subject: [PATCH] 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"]); }); });