diff --git a/CONTEXT.md b/CONTEXT.md index 9d42611ef..f74189512 100644 --- a/CONTEXT.md +++ b/CONTEXT.md @@ -54,7 +54,7 @@ The classification of a Vertex Type's configured icon into what it takes to rend _Avoid_: Icon type (ambiguous with `iconImageType`, the stored MIME string) **Icon Registry**: -The single store of resolved icons, keyed by Icon Source Id and shared by every Icon Surface. Holds a color-free artifact — a sanitized SVG string or a raster url — so applying a Vertex Type's color stays a pure transform at the point of use. A plain external store outside React/Jotai, bridged by `useSyncExternalStore`; explicitly **not** TanStack Query, because a per-hook subscription scaled with Vertex Type count and locked up the Schema View at 10k. Resolves a raster url synchronously, allows a failed icon three attempts in total, and never stores a failure as a result. See `docs/adr/20260813-icon-registry-not-react-query.md`. +The single store of resolved icons, keyed by Icon Source Id and shared by every Icon Surface. Holds a color-free artifact — a sanitized SVG string or a raster url, plus its natural width/height so a non-square icon scales instead of stretching — so applying a Vertex Type's color stays a pure transform at the point of use. A plain external store outside React/Jotai, bridged by `useSyncExternalStore`; explicitly **not** TanStack Query, because a per-hook subscription scaled with Vertex Type count and locked up the Schema View at 10k. Every icon kind resolves asynchronously (a raster's dimensions are measured by loading it), allows a failed icon three attempts in total, and never stores a failure as a result. See `docs/adr/20260813-icon-registry-not-react-query.md`. _Avoid_: Icon cache (it is the source of truth for resolution, not a layer in front of one) **Icon Surface**: diff --git a/packages/graph-explorer/src/components/VertexIcon.tsx b/packages/graph-explorer/src/components/VertexIcon.tsx index bea91d8a2..99bd72340 100644 --- a/packages/graph-explorer/src/components/VertexIcon.tsx +++ b/packages/graph-explorer/src/components/VertexIcon.tsx @@ -3,13 +3,16 @@ import { DynamicIcon } from "lucide-react/dynamic"; import SVG from "react-inlinesvg"; import { useVertexStyle, type VertexStyle, type VertexType } from "@/core"; +import { ensureSvgViewBox, SVG_ALLOWED_ATTR } from "@/core/icons"; import { cn } from "@/utils"; import { getLucideName, isValidLucideIconName } from "@/utils/lucideIcons"; function sanitizeSvg(svg: string): string { - return DOMPurify.sanitize(svg, { + const sanitized = DOMPurify.sanitize(svg, { USE_PROFILES: { svg: true, svgFilters: true }, + ALLOWED_ATTR: SVG_ALLOWED_ATTR, }); + return ensureSvgViewBox(sanitized); } interface Props { @@ -54,7 +57,7 @@ function VertexIcon({ vertexStyle, className, alt }: Props) { {altText} ); diff --git a/packages/graph-explorer/src/core/icons/aspectFit.test.ts b/packages/graph-explorer/src/core/icons/aspectFit.test.ts new file mode 100644 index 000000000..da7bec25d --- /dev/null +++ b/packages/graph-explorer/src/core/icons/aspectFit.test.ts @@ -0,0 +1,17 @@ +import { describe, expect, it } from "vitest"; + +import { fitAspectRatio } from "./aspectFit"; + +describe("fitAspectRatio", () => { + it("caps the wider axis at base and shrinks the shorter one proportionally", () => { + expect(fitAspectRatio(400, 100, 24)).toEqual([24, 6]); + }); + + it("caps the taller axis at base and shrinks the shorter one proportionally", () => { + expect(fitAspectRatio(100, 400, 24)).toEqual([6, 24]); + }); + + it("returns the base for both axes when already square", () => { + expect(fitAspectRatio(100, 100, 24)).toEqual([24, 24]); + }); +}); diff --git a/packages/graph-explorer/src/core/icons/aspectFit.ts b/packages/graph-explorer/src/core/icons/aspectFit.ts new file mode 100644 index 000000000..a09c18581 --- /dev/null +++ b/packages/graph-explorer/src/core/icons/aspectFit.ts @@ -0,0 +1,16 @@ +/** + * Scales a width/height pair to fit a `base`-sized box, preserving aspect + * ratio: the longer axis becomes exactly `base`, the shorter one shrinks + * proportionally. Returns raw numbers — each caller formats its own unit + * (an absolute pixel size, a cytoscape percentage, ...). + */ +export function fitAspectRatio( + width: number, + height: number, + base: number, +): [width: number, height: number] { + const aspectRatio = width / height; + return aspectRatio >= 1 + ? [base, base / aspectRatio] + : [base * aspectRatio, base]; +} diff --git a/packages/graph-explorer/src/core/icons/iconImageUrl.test.ts b/packages/graph-explorer/src/core/icons/iconImageUrl.test.ts index 632ae8792..7652b684e 100644 --- a/packages/graph-explorer/src/core/icons/iconImageUrl.test.ts +++ b/packages/graph-explorer/src/core/icons/iconImageUrl.test.ts @@ -103,4 +103,40 @@ describe("toIconImageUrl", () => { expect(red).not.toBe(blue); }); + + // Issue #2108: forcing every icon's intrinsic size to a fixed 24x24 square + // bakes a mismatched-aspect letterbox into the rasterized image, which the + // consumer's own aspect-aware background-width/height then stretches a + // second time — distorting a non-square icon worse than doing nothing. + describe("non-square icons (issue #2108)", () => { + const WIDE_SVG = ``; + const TALL_SVG = ``; + + it("scales a wide icon's intrinsic width/height to its real aspect ratio", () => { + const result = toIconImageUrl( + { kind: "svg", svg: WIDE_SVG, width: 400, height: 100 }, + "#FF0000", + ); + + expect(decode(result)).toContain('width="24"'); + expect(decode(result)).toContain('height="6"'); + }); + + it("scales a tall icon's intrinsic width/height to its real aspect ratio", () => { + const result = toIconImageUrl( + { kind: "svg", svg: TALL_SVG, width: 100, height: 400 }, + "#FF0000", + ); + + expect(decode(result)).toContain('width="6"'); + expect(decode(result)).toContain('height="24"'); + }); + + it("falls back to a 24x24 square when dimensions are unknown", () => { + const result = toIconImageUrl({ kind: "svg", svg: WIDE_SVG }, "#FF0000"); + + expect(decode(result)).toContain('width="24"'); + expect(decode(result)).toContain('height="24"'); + }); + }); }); diff --git a/packages/graph-explorer/src/core/icons/iconImageUrl.ts b/packages/graph-explorer/src/core/icons/iconImageUrl.ts index 64e9f1d08..261e9c2cc 100644 --- a/packages/graph-explorer/src/core/icons/iconImageUrl.ts +++ b/packages/graph-explorer/src/core/icons/iconImageUrl.ts @@ -1,7 +1,9 @@ import type { ResolvedIcon } from "./iconRegistry"; -/** Intrinsic size; both consumers scale from it. Matches the cytoscape node size. */ -const ICON_SIZE = "24"; +import { fitAspectRatio } from "./aspectFit"; + +/** Intrinsic size baseline; matches the cytoscape node size. */ +const ICON_SIZE = 24; /** * Pure transform to an image url. @@ -16,19 +18,44 @@ export function toIconImageUrl(icon: ResolvedIcon, color: string): string { case "raster": return icon.url; case "svg": - return encodeSvg(applySizeAndColor(icon.svg, color)); + return encodeSvg( + applySizeAndColor(icon.svg, color, icon.width, icon.height), + ); } } -function applySizeAndColor(svgContent: string, color: string): string { +/** + * Sets the SVG's own intrinsic width/height. This must preserve the icon's + * real aspect ratio (scaled to fit a 24px box), not force a fixed square: + * forcing a square here bakes a mismatched-aspect letterbox into the + * rasterized image, which the consumer's own aspect-aware background-width/ + * height then stretches a second time, distorting worse than doing nothing. + */ +function applySizeAndColor( + svgContent: string, + color: string, + naturalWidth?: number, + naturalHeight?: number, +): string { const doc = new DOMParser().parseFromString(svgContent, "application/xml"); const root = doc.documentElement; - root.setAttribute("width", ICON_SIZE); - root.setAttribute("height", ICON_SIZE); + const [width, height] = fitToIconSize(naturalWidth, naturalHeight); + root.setAttribute("width", String(width)); + root.setAttribute("height", String(height)); applyColor(root, color); return new XMLSerializer().serializeToString(root); } +function fitToIconSize( + naturalWidth?: number, + naturalHeight?: number, +): [width: number, height: number] { + if (!naturalWidth || !naturalHeight) { + return [ICON_SIZE, ICON_SIZE]; + } + return fitAspectRatio(naturalWidth, naturalHeight, ICON_SIZE); +} + /** * Sets `color` on the root so `currentColor`-authored icons follow the vertex * color by inheritance; hardcoded fills are left untouched. Isolated here so diff --git a/packages/graph-explorer/src/core/icons/iconRegistry.test.ts b/packages/graph-explorer/src/core/icons/iconRegistry.test.ts index 644b85777..200c73368 100644 --- a/packages/graph-explorer/src/core/icons/iconRegistry.test.ts +++ b/packages/graph-explorer/src/core/icons/iconRegistry.test.ts @@ -49,7 +49,7 @@ describe("iconRegistry", () => { iconRegistry.request([source]); await settle(); - expect(iconRegistry.getSnapshot().get(iconSourceId(source)!)).toStrictEqual( + expect(iconRegistry.getSnapshot().get(iconSourceId(source)!)).toMatchObject( { kind: "raster", url: "https://example.test/a.png", @@ -58,18 +58,23 @@ describe("iconRegistry", () => { expect(fetch).not.toBeCalled(); }); - // A url needs no resolution, so making the consumer wait a render for it - // would be a pointless async round trip. - it("resolves a raster icon synchronously", () => { + // Measuring a raster's natural size requires loading it, so — unlike a url, + // which needs no resolution — this can no longer settle in the same tick. + it("measures a raster icon's natural dimensions", async () => { const source = classifyIconSource({ iconUrl: "https://example.test/a.png", iconImageType: "image/png", }); iconRegistry.request([source]); + await settle(); - expect(iconRegistry.getSnapshot().has(iconSourceId(source)!)).toBe(true); - expect(iconRegistry.pendingCount).toBe(0); + expect(iconRegistry.getSnapshot().get(iconSourceId(source)!)).toMatchObject( + { + width: expect.any(Number), + height: expect.any(Number), + }, + ); }); it("fetches and sanitizes a remote svg", async () => { diff --git a/packages/graph-explorer/src/core/icons/iconRegistry.ts b/packages/graph-explorer/src/core/icons/iconRegistry.ts index e4ed001dc..486861bb3 100644 --- a/packages/graph-explorer/src/core/icons/iconRegistry.ts +++ b/packages/graph-explorer/src/core/icons/iconRegistry.ts @@ -4,11 +4,13 @@ import { logger } from "@/utils"; import { getLucideSvgString } from "@/utils/lucideIcons"; import { type IconSource, type IconSourceId, iconSourceId } from "./iconSource"; +import { SVG_ALLOWED_ATTR } from "./svgSanitize"; +import { ensureSvgViewBox } from "./svgViewBox"; /** An icon resolved to a renderable form, with no color applied yet. */ export type ResolvedIcon = - | { kind: "raster"; url: string } - | { kind: "svg"; svg: string }; + | { kind: "raster"; url: string; width?: number; height?: number } + | { kind: "svg"; svg: string; width?: number; height?: number }; /** * Bounded so a permanently broken icon stops re-fetching, but not one-shot: a @@ -48,30 +50,17 @@ class IconRegistry { /** Idempotent: starts only what is neither resolved, running, nor exhausted. */ request(sources: Iterable): void { - let next: Map | undefined; - for (const source of sources) { const id = iconSourceId(source); if (id === null || this.#resolved.has(id) || this.#inFlight.has(id)) { continue; } - if (source.kind === "raster") { - // A url needs no work, so resolve it now rather than a render later. - next ??= new Map(this.#resolved); - next.set(id, { kind: "raster", url: source.url }); - continue; - } if ((this.#failures.get(id) ?? 0) >= MAX_ATTEMPTS) { continue; } this.#inFlight.add(id); void this.#resolve(id, source, this.#epoch); } - - if (next) { - this.#resolved = next; - this.#notify(); - } } /** @@ -139,29 +128,97 @@ async function resolveIconSource( switch (source.kind) { case "none": return null; - case "raster": - return { kind: "raster", url: source.url }; + case "raster": { + const dimensions = await measureImageDimensions(source.url); + return { kind: "raster", url: source.url, ...dimensions }; + } case "lucide": { - const svg = await getLucideSvgString(source.name); - if (svg === null) { + const raw = await getLucideSvgString(source.name); + if (raw === null) { logger.warn("Unknown lucide icon", source.name); return null; } - return { kind: "svg", svg }; + const svg = ensureSvgViewBox(raw); + const dimensions = extractSvgDimensions(svg); + return { kind: "svg", svg, ...dimensions }; } case "svg": { // Untrusted: a user-supplied SVG, sanitized before it is used anywhere. const response = await fetch(source.url); - const svg = DOMPurify.sanitize(await response.text(), { + const sanitized = DOMPurify.sanitize(await response.text(), { USE_PROFILES: { svg: true, svgFilters: true }, + ALLOWED_ATTR: SVG_ALLOWED_ATTR, }); // A 404 body sanitizes to something that is not SVG. Reject it here so // consumers can treat `ResolvedIcon` as renderable. - return isParseableSvg(svg) ? { kind: "svg", svg } : null; + if (!isParseableSvg(sanitized)) { + return null; + } + const svg = ensureSvgViewBox(sanitized); + const dimensions = extractSvgDimensions(svg); + return { kind: "svg", svg, ...dimensions }; } } } +async function measureImageDimensions( + url: string, +): Promise<{ width?: number; height?: number }> { + try { + // Never rejects — `onerror` resolves to a fallback instead. This only + // guards a synchronous throw from constructing `Image` or setting `src`. + return await new Promise(resolve => { + const img = new Image(); + img.onload = () => { + resolve({ width: img.naturalWidth, height: img.naturalHeight }); + }; + img.onerror = () => { + resolve({}); + }; + img.src = url; + }); + } catch (_e) { + return {}; + } +} + +function extractSvgDimensions(svg: string): { + width?: number; + height?: number; +} { + try { + const doc = new DOMParser().parseFromString(svg, "application/xml"); + const root = doc.documentElement; + + if (root.localName !== "svg") { + return {}; + } + + const width = parseFloat(root.getAttribute("width") ?? ""); + const height = parseFloat(root.getAttribute("height") ?? ""); + + if (!isNaN(width) && !isNaN(height)) { + return { width, height }; + } + + const viewBox = root.getAttribute("viewBox"); + if (viewBox) { + const parts = viewBox.split(/\s+/); + if (parts.length >= 4) { + const vbWidth = parseFloat(parts[2]); + const vbHeight = parseFloat(parts[3]); + if (!isNaN(vbWidth) && !isNaN(vbHeight)) { + return { width: vbWidth, height: vbHeight }; + } + } + } + + return {}; + } catch (_e) { + return {}; + } +} + function isParseableSvg(svg: string): boolean { const doc = new DOMParser().parseFromString(svg, "application/xml"); return ( diff --git a/packages/graph-explorer/src/core/icons/iconSurfaces.test.tsx b/packages/graph-explorer/src/core/icons/iconSurfaces.test.tsx index 4db651a74..3328e50dd 100644 --- a/packages/graph-explorer/src/core/icons/iconSurfaces.test.tsx +++ b/packages/graph-explorer/src/core/icons/iconSurfaces.test.tsx @@ -73,4 +73,37 @@ describe("icon resolution across surfaces", () => { // One additional fetch for the new icon, not two for the whole set. expect(fetch).toBeCalledTimes(2); }); + + // Issue #2108: a non-square custom icon (a wide logo, say) must keep its + // aspect ratio on the canvas rather than being squashed into a square. + // The uploaded SVG has width/height but no viewBox — exactly what a plain + // `` export produces — so this also covers viewBox + // synthesis end to end, not just the aspect-ratio math in isolation. + it("computes aspect-ratio-preserving background dimensions for a wide custom icon", async () => { + vi.stubGlobal( + "fetch", + vi.fn(() => + Promise.resolve( + new Response( + ``, + ), + ), + ), + ); + + const canvas = renderHook(() => + useBackgroundImageMap([ + style({ + type: createVertexType("Wide"), + iconUrl: "https://example.test/wide-logo.svg", + }), + ]), + ); + await waitFor(() => expect(canvas.result.current.size).toBe(1)); + + const imageData = canvas.result.current.get(createVertexType("Wide"))!; + expect(imageData.width).toBe("60%"); + // 60% / (400/100) = 15%, not the 60% a square icon would get. + expect(imageData.height).toBe("15.0%"); + }); }); diff --git a/packages/graph-explorer/src/core/icons/index.ts b/packages/graph-explorer/src/core/icons/index.ts index 5308889c1..d4c56efeb 100644 --- a/packages/graph-explorer/src/core/icons/index.ts +++ b/packages/graph-explorer/src/core/icons/index.ts @@ -1,4 +1,7 @@ +export * from "./aspectFit"; export * from "./iconImageUrl"; export * from "./iconRegistry"; export * from "./iconSource"; +export * from "./svgSanitize"; +export * from "./svgViewBox"; export * from "./useResolvedIcons"; diff --git a/packages/graph-explorer/src/core/icons/svgSanitize.ts b/packages/graph-explorer/src/core/icons/svgSanitize.ts new file mode 100644 index 000000000..34e8bd29b --- /dev/null +++ b/packages/graph-explorer/src/core/icons/svgSanitize.ts @@ -0,0 +1,32 @@ +/** + * DOMPurify's default SVG profile drops `width`/`height`/`viewBox` — safe + * attributes it doesn't allowlist by default — which silently breaks aspect + * ratio for every sanitized icon. Every consumer that sanitizes an untrusted + * SVG needs this same allowlist, so it lives here once rather than drifting + * between the DOM and canvas render paths. + */ +export const SVG_ALLOWED_ATTR = [ + "width", + "height", + "viewBox", + "xmlns", + "preserveAspectRatio", + "fill", + "stroke", + "id", + "class", + "style", + "x", + "y", + "x1", + "y1", + "x2", + "y2", + "cx", + "cy", + "r", + "offset", + "stop-color", + "stop-opacity", + "opacity", +]; diff --git a/packages/graph-explorer/src/core/icons/svgViewBox.test.ts b/packages/graph-explorer/src/core/icons/svgViewBox.test.ts new file mode 100644 index 000000000..ee8ee5a79 --- /dev/null +++ b/packages/graph-explorer/src/core/icons/svgViewBox.test.ts @@ -0,0 +1,34 @@ +// @vitest-environment jsdom + +// DEV NOTE: happy-dom's DOMParser is not reliable for the svg render path. + +import { describe, expect, it } from "vitest"; + +import { ensureSvgViewBox } from "./svgViewBox"; + +describe("ensureSvgViewBox", () => { + // Issue #2108: without a viewBox, an SVG has no coordinate system to scale + // from — forcing a different width/height on the root just clips the + // content instead of scaling it. + it("synthesizes a viewBox from width/height when one is missing", () => { + const svg = ``; + + expect(ensureSvgViewBox(svg)).toContain('viewBox="0 0 400 100"'); + }); + + it("leaves an existing viewBox untouched", () => { + const svg = ``; + + expect(ensureSvgViewBox(svg)).toBe(svg); + }); + + it("leaves the svg untouched when width/height are also missing", () => { + const svg = ``; + + expect(ensureSvgViewBox(svg)).toBe(svg); + }); + + it("leaves non-svg or unparseable input untouched", () => { + expect(ensureSvgViewBox("not xml at all <<<")).toBe("not xml at all <<<"); + }); +}); diff --git a/packages/graph-explorer/src/core/icons/svgViewBox.ts b/packages/graph-explorer/src/core/icons/svgViewBox.ts new file mode 100644 index 000000000..7843cc572 --- /dev/null +++ b/packages/graph-explorer/src/core/icons/svgViewBox.ts @@ -0,0 +1,30 @@ +/** + * Ensures an SVG has a `viewBox`, synthesizing one from `width`/`height` when + * absent. + * + * Without a `viewBox`, an SVG has no internal coordinate system to scale from: + * forcing a different CSS or attribute size on the root just clips the content + * to the new box instead of scaling it (`preserveAspectRatio` has nothing to + * map). A synthesized `viewBox="0 0 "` gives the renderer that + * mapping, so resizing scales instead of crops. + */ +export function ensureSvgViewBox(svg: string): string { + try { + const doc = new DOMParser().parseFromString(svg, "application/xml"); + const root = doc.documentElement; + if (root.localName !== "svg" || root.hasAttribute("viewBox")) { + return svg; + } + + const width = parseFloat(root.getAttribute("width") ?? ""); + const height = parseFloat(root.getAttribute("height") ?? ""); + if (isNaN(width) || isNaN(height) || width <= 0 || height <= 0) { + return svg; + } + + root.setAttribute("viewBox", `0 0 ${width} ${height}`); + return new XMLSerializer().serializeToString(root); + } catch { + return svg; + } +} diff --git a/packages/graph-explorer/src/modules/GraphViewer/useBackgroundImageMap.test.ts b/packages/graph-explorer/src/modules/GraphViewer/useBackgroundImageMap.test.ts index f15338e94..bad779573 100644 --- a/packages/graph-explorer/src/modules/GraphViewer/useBackgroundImageMap.test.ts +++ b/packages/graph-explorer/src/modules/GraphViewer/useBackgroundImageMap.test.ts @@ -51,13 +51,43 @@ describe("useBackgroundImageMap", () => { const { result } = renderMap([config]); await waitFor(() => - expect(result.current.get(createVertexType("Raster"))).toBe( + expect(result.current.get(createVertexType("Raster"))?.url).toBe( "https://example.test/a.png", ), ); expect(fetch).not.toBeCalled(); }); + // Issue #2108: a non-square raster (not just SVG) must keep its aspect + // ratio too. setupTests.ts's global Image double always measures 24x24, so + // this overrides it for one test to prove a real wide/tall raster result. + it("computes aspect-ratio-preserving dimensions for a non-square raster", async () => { + class WideImage { + onload: (() => void) | null = null; + naturalWidth = 400; + naturalHeight = 100; + set src(_value: string) { + queueMicrotask(() => this.onload?.()); + } + } + vi.stubGlobal("Image", WideImage); + + const config = makeConfig({ + type: createVertexType("WideRaster"), + iconUrl: "https://example.test/wide.png", + iconImageType: "image/png", + }); + + const { result } = renderMap([config]); + + await waitFor(() => + expect(result.current.get(createVertexType("WideRaster"))).toMatchObject({ + width: "60%", + height: "15.0%", + }), + ); + }); + it("styles a fetched svg into a data uri", async () => { const config = makeConfig({ type: createVertexType("Svg"), @@ -71,9 +101,9 @@ describe("useBackgroundImageMap", () => { await waitFor(() => expect(result.current.has(createVertexType("Svg"))).toBe(true), ); - const value = result.current.get(createVertexType("Svg"))!; - expect(value.startsWith("data:image/svg+xml;utf8,")).toBe(true); - expect(decodeURIComponent(value)).toContain("color:#FF0000"); + const imageData = result.current.get(createVertexType("Svg"))!; + expect(imageData.url.startsWith("data:image/svg+xml;utf8,")).toBe(true); + expect(decodeURIComponent(imageData.url)).toContain("color:#FF0000"); }); it("styles a lucide icon into a data uri carrying the node color", async () => { @@ -89,9 +119,9 @@ describe("useBackgroundImageMap", () => { await waitFor(() => expect(result.current.has(createVertexType("Lucide"))).toBe(true), ); - const value = result.current.get(createVertexType("Lucide"))!; - expect(value.startsWith("data:image/svg+xml;utf8,")).toBe(true); - expect(decodeURIComponent(value)).toContain("color:#00FF00"); + const imageData = result.current.get(createVertexType("Lucide"))!; + expect(imageData.url.startsWith("data:image/svg+xml;utf8,")).toBe(true); + expect(decodeURIComponent(imageData.url)).toContain("color:#00FF00"); }); it("omits configs with no icon and unresolvable icons", async () => { @@ -135,10 +165,10 @@ describe("useBackgroundImageMap", () => { await waitFor(() => expect(result.current.size).toBe(2)); expect( - decodeURIComponent(result.current.get(createVertexType("Red"))!), + decodeURIComponent(result.current.get(createVertexType("Red"))!.url), ).toContain("color:#FF0000"); expect( - decodeURIComponent(result.current.get(createVertexType("Blue"))!), + decodeURIComponent(result.current.get(createVertexType("Blue"))!.url), ).toContain("color:#0000FF"); // One icon identity, so one fetch — color is applied by a pure transform. expect(fetch).toBeCalledTimes(1); diff --git a/packages/graph-explorer/src/modules/GraphViewer/useBackgroundImageMap.ts b/packages/graph-explorer/src/modules/GraphViewer/useBackgroundImageMap.ts index 1a52a156e..40a986161 100644 --- a/packages/graph-explorer/src/modules/GraphViewer/useBackgroundImageMap.ts +++ b/packages/graph-explorer/src/modules/GraphViewer/useBackgroundImageMap.ts @@ -2,15 +2,23 @@ import type { VertexStyle, VertexType } from "@/core"; import { classifyIconSource, + fitAspectRatio, type IconSource, type IconSourceId, iconSourceId, + type ResolvedIcon, toIconImageUrl, useResolvedIcons, } from "@/core/icons"; +export interface BackgroundImageData { + url: string; + width: string; + height: string; +} + /** - * Maps each vertex type to its cytoscape `background-image`. + * Maps each vertex type to its cytoscape `background-image` with aspect-ratio-aware dimensions. * * The set of UNIQUE icons is tiny (dozens) even with thousands of vertex types, * so resolution is keyed by icon identity and shared through the icon registry. @@ -18,7 +26,7 @@ import { */ export function useBackgroundImageMap( vtConfigs: VertexStyle[], -): Map { +): Map { // Single pass: this runs on every render over every vertex type, so each // config is classified once and the id is reused for both lookups below. const uniqueSources = new Map(); @@ -41,20 +49,45 @@ export function useBackgroundImageMap( const icons = useResolvedIcons([...uniqueSources.values()]); - const result = new Map(); - const rendered = new Map(); + const result = new Map(); + const rendered = new Map(); for (const { type, id, color } of identified) { const icon = icons.get(id); if (!icon) { continue; } - const renderKey = `${id}\u0000${color}`; - let backgroundImage = rendered.get(renderKey); - if (backgroundImage === undefined) { - backgroundImage = toIconImageUrl(icon, color); - rendered.set(renderKey, backgroundImage); + const renderKey = `${id}|${color}`; + let imageData = rendered.get(renderKey); + if (imageData === undefined) { + const url = toIconImageUrl(icon, color); + const { width, height } = computeAspectRatioAwareDimensions(icon); + imageData = { url, width, height }; + rendered.set(renderKey, imageData); } - result.set(type, backgroundImage); + result.set(type, imageData); } return result; } + +const BASE_PERCENT = 60; + +function computeAspectRatioAwareDimensions(icon: ResolvedIcon): { + width: string; + height: string; +} { + if (!icon.width || !icon.height) { + return { width: "60%", height: "60%" }; + } + + const [width, height] = fitAspectRatio(icon.width, icon.height, BASE_PERCENT); + return { + width: toPercent(width), + height: toPercent(height), + }; +} + +// The untouched axis stays an exact "60%" rather than "60.0%", matching the +// existing default so this is a no-op change in style output for square icons. +function toPercent(value: number): string { + return value === BASE_PERCENT ? "60%" : `${value.toFixed(1)}%`; +} diff --git a/packages/graph-explorer/src/modules/GraphViewer/useGraphStyles.test.tsx b/packages/graph-explorer/src/modules/GraphViewer/useGraphStyles.test.tsx index 33da0ec8a..d55bc5400 100644 --- a/packages/graph-explorer/src/modules/GraphViewer/useGraphStyles.test.tsx +++ b/packages/graph-explorer/src/modules/GraphViewer/useGraphStyles.test.tsx @@ -59,6 +59,11 @@ describe("useGraphStyles", () => { "background-image": RASTER_ICON.iconUrl, "background-color": "#128EE5", "background-opacity": 0.8, + // Aspect-ratio-aware sizing (issue #2108): square by default since + // the test double measures every raster icon as 24x24. + "background-fit": "none", + "background-width": "60%", + "background-height": "60%", "border-color": "#000000", "border-width": 2, "border-opacity": 1, diff --git a/packages/graph-explorer/src/modules/GraphViewer/useGraphStyles.ts b/packages/graph-explorer/src/modules/GraphViewer/useGraphStyles.ts index 869ded042..a3341ffe8 100644 --- a/packages/graph-explorer/src/modules/GraphViewer/useGraphStyles.ts +++ b/packages/graph-explorer/src/modules/GraphViewer/useGraphStyles.ts @@ -11,7 +11,10 @@ import { type VertexType, } from "@/core"; -import { useBackgroundImageMap } from "./useBackgroundImageMap"; +import { + useBackgroundImageMap, + type BackgroundImageData, +} from "./useBackgroundImageMap"; const LINE_PATTERN = { solid: undefined, @@ -38,19 +41,22 @@ export default function useGraphStyles() { function createGraphStyles( deferredVtConfigs: VertexStyle[], deferredEtConfigs: EdgeStyle[], - backgroundImageMap: Map, + backgroundImageMap: Map, ): GraphProps["styles"] { const styles: GraphProps["styles"] = {}; for (const vtConfig of deferredVtConfigs) { const vt = vtConfig.type; - const backgroundImage = backgroundImageMap.get(vt); + const imageData = backgroundImageMap.get(vt); styles[`node[type="${vt}"]`] = { - "background-image": backgroundImage, + "background-image": imageData?.url, "background-color": vtConfig.color, "background-opacity": vtConfig.backgroundOpacity, + "background-width": imageData?.width, + "background-height": imageData?.height, + "background-fit": "none", "border-color": vtConfig.borderColor, "border-width": vtConfig.borderWidth, "border-opacity": vtConfig.borderWidth > 0 ? 1 : 0, diff --git a/packages/graph-explorer/src/setupTests.ts b/packages/graph-explorer/src/setupTests.ts index 60a65f807..75803631c 100644 --- a/packages/graph-explorer/src/setupTests.ts +++ b/packages/graph-explorer/src/setupTests.ts @@ -14,6 +14,24 @@ import { iconRegistry } from "@/core/icons"; expect.extend(matchers); +/** + * jsdom never actually decodes images, so a real `Image` never fires + * `onload`/`onerror` — raster icon dimension measurement would hang every + * test that resolves one. This double fires `onload` on the next microtask + * with a fixed square size, matching the pre-measurement fallback so + * existing assertions about square icons stay valid. + */ +class MockImage { + onload: (() => void) | null = null; + onerror: (() => void) | null = null; + naturalWidth = 24; + naturalHeight = 24; + + set src(_value: string) { + queueMicrotask(() => this.onload?.()); + } +} + // Mock getAppStore to return a specific test store let store = createStore(); vi.mock(import("@/core/StateProvider/appStore"), () => { @@ -30,6 +48,9 @@ beforeEach(async () => { store = createStore(); vi.stubEnv("DEV", true); vi.stubEnv("PROD", false); + // Re-stubbed every test: a test file's own afterEach may call + // `vi.unstubAllGlobals()`, which would otherwise wipe this after its first test. + vi.stubGlobal("Image", MockImage); // The icon registry is a module singleton, so resolved icons would otherwise // bleed between tests.