fix(react): namespace SVG resource IDs per instance

.changeset/per-instance-svg-ids.md

@@ -0,0 +1,5 @@
+---
+'@bgd-labs/icons-react': patch
+---
+
+Namespace SVG resource ids per mounted instance. Icon SVGs carry static ids (SVGO's per-asset prefix, e.g. `weth_full__clip0_…`), so two mounted copies of the same icon shared clipPath/gradient ids — and since `url(#…)` resolves document-wide to the first match, a copy sitting in a hidden subtree (a closed drawer, a `display:none` panel) hijacked the references of every visible copy and blanked it. Generated components that define ids now derive a per-instance prefix from `useId()` (SSR-stable, sanitized to CSS-url-safe characters) and bake it into ids and every `url(#)`/`href` reference; id-free icons are emitted unchanged, with no `useId` call. The GitHub network fallback applies the same prefixing at conversion time, and only shares its cached converted content across mounts for id-free SVGs.

packages/react/src/__tests__/instance-ids.test.tsx

@@ -0,0 +1,53 @@
+// @vitest-environment jsdom
+import { afterEach, describe, expect, it } from 'vitest'
+import { cleanup, render } from '@testing-library/react'
+// WETH's full art defines a clipPath — the generated component must give
+// every mounted instance its own ids. (If the asset ever loses its clipPath,
+// swap in any token whose _full.svg carries a <defs> id.)
+import WethIconFull from '../generated/tokens/weth.full'
+// MEGA's full art defines no ids — the id-free fast path (no useId call).
+import MegaIconFull from '../generated/tokens/mega.full'
+
+afterEach(cleanup)
+
+// SVG id lookups (url(#...)) are document-global and take the FIRST match:
+// two mounted copies of one icon sharing ids means one copy's refs resolve
+// into the other — and when that other sits in a hidden subtree (a closed
+// drawer, a display:none panel), the visible icon clips to nothing.
+describe('generated components parameterize resource ids per instance', () => {
+  it('two instances of the same icon get distinct ids, each self-referencing', () => {
+    const { container } = render(
+      <>
+        <WethIconFull data-testid="a" />
+        <WethIconFull data-testid="b" />
+      </>,
+    )
+    const svgs = Array.from(container.querySelectorAll('svg'))
+    expect(svgs).toHaveLength(2)
+
+    const details = svgs.map((svg) => {
+      const clip = svg.querySelector('clipPath')
+      const ref = svg.querySelector('[clip-path]')?.getAttribute('clip-path')
+      return { id: clip?.id, ref }
+    })
+
+    for (const { id, ref } of details) {
+      expect(id).toBeTruthy()
+      // Each instance references its OWN clipPath.
+      expect(ref).toBe(`url(#${id})`)
+      // The prefix must stay CSS-url safe: React's useId delimiters
+      // (":" / "«»") are stripped by the generated sanitize.
+      expect(id).toMatch(/^[a-zA-Z0-9_-]+$/)
+      // The asset-level SVGO prefix is still part of the id.
+      expect(id).toContain('weth_full__')
+    }
+    expect(details[0].id).not.toBe(details[1].id)
+  })
+
+  it('id-free icons render without useId plumbing artifacts', () => {
+    const { container } = render(<MegaIconFull />)
+    const svg = container.querySelector('svg')
+    expect(svg).not.toBeNull()
+    expect(svg!.querySelectorAll('[id]')).toHaveLength(0)
+  })
+})

packages/react/src/__tests__/svg-to-react.test.tsx

@@ -77,3 +77,53 @@ describe('svgTextToReact', () => {
     expect(g?.querySelector('rect')).not.toBeNull()
   })
 })
+
+describe('svgTextToReact id prefixing', () => {
+  const svgWithIds = wrap(
+    '<g clip-path="url(#a_clip)"><rect width="32" height="32" fill="url(#a_grad)"/>' +
+      '<circle style="fill:url(#a_grad)" cx="16" cy="16" r="8"/></g>' +
+      '<defs><clipPath id="a_clip"><path d="M0 0h32v32H0z"/></clipPath>' +
+      '<linearGradient id="a_grad"/></defs>',
+  )
+
+  it('reports hasIds and rewrites ids and their references with the prefix', () => {
+    const content = svgTextToReact(svgWithIds, 'p1-')
+    expect(content?.hasIds).toBe(true)
+    const { container } = render(
+      <svg viewBox={content!.viewBox}>{content!.node}</svg>,
+    )
+    expect(container.querySelector('clipPath')?.id).toBe('p1-a_clip')
+    expect(container.querySelector('linearGradient')?.id).toBe('p1-a_grad')
+    expect(container.querySelector('g')?.getAttribute('clip-path')).toBe(
+      'url(#p1-a_clip)',
+    )
+    expect(container.querySelector('rect')?.getAttribute('fill')).toBe(
+      'url(#p1-a_grad)',
+    )
+    expect(
+      (container.querySelector('circle') as SVGElement).style.fill,
+    ).toContain('url(#p1-a_grad')
+  })
+
+  it('leaves references to undefined ids and plain color values alone', () => {
+    const content = svgTextToReact(
+      wrap(
+        '<rect fill="#fff" filter="url(#external)" width="32" height="32"/>' +
+          '<defs><clipPath id="own"><path d="M0 0h1v1H0z"/></clipPath></defs>',
+      ),
+      'p2-',
+    )
+    const { container } = render(
+      <svg viewBox={content!.viewBox}>{content!.node}</svg>,
+    )
+    const rect = container.querySelector('rect')
+    expect(rect?.getAttribute('fill')).toBe('#fff')
+    expect(rect?.getAttribute('filter')).toBe('url(#external)')
+    expect(container.querySelector('clipPath')?.id).toBe('p2-own')
+  })
+
+  it('reports hasIds false for id-free SVGs', () => {
+    const content = svgTextToReact(wrap('<circle cx="16" cy="16" r="8"/>'))
+    expect(content?.hasIds).toBe(false)
+  })
+})

packages/react/src/github-fallback.tsx

@@ -1,4 +1,4 @@
-import { useEffect, useMemo, useState } from 'react'
+import { useEffect, useId, useMemo, useState } from 'react'
 import type { ReactNode, SVGProps } from 'react'
 import { resolve } from '@bgd-labs/icons/resolve'
 import { useIconConfig } from './icon-provider'
@@ -10,7 +10,9 @@ import type { IconType } from './types'
 // `content` memoises the converted React description so repeated mounts of
 // the same fallback icon reuse one sanitize/parse pass instead of re-running
 // it per instance. React elements are immutable, so sharing one
-// SvgReactContent across many component instances is safe.
+// SvgReactContent across many component instances is safe — but ONLY for
+// id-free SVGs: converted content that defines resource ids carries one
+// instance's id prefix and is never cached (see cachedContentFor).
 interface CacheEntry {
   value: string | null
   ts: number
@@ -54,12 +56,19 @@ function cacheSet(url: string, value: string | null) {
 
 // Memoise the converted content on a positive cache entry. Only non-null
 // conversions are stored (SSR returns null when DOMParser is missing — never
-// cache that). Returns the converted content, or null when unconvertible.
-function cachedContentFor(url: string, text: string): SvgReactContent | null {
+// cache that), and only when the SVG defines no resource ids: ids get the
+// caller's per-instance prefix baked in, so that conversion belongs to one
+// mount and each instance converts its own copy. Returns the converted
+// content, or null when unconvertible.
+function cachedContentFor(
+  url: string,
+  text: string,
+  idPrefix: string,
+): SvgReactContent | null {
   const entry = svgCache.get(url)
   if (entry && entry.content) return entry.content
-  const content = svgTextToReact(text)
-  if (content && entry) entry.content = content
+  const content = svgTextToReact(text, idPrefix)
+  if (content && entry && !content.hasIds) entry.content = content
   return content
 }
 
@@ -189,11 +198,14 @@ function getCachedContent(
   type: AssetType | null,
   id: string,
   variant: string,
+  idPrefix: string,
 ): SvgReactContent | null {
   if (!type) return null
   const url = buildUrl(baseUrl, branch, type, id, variant)
   const cached = cacheGet(url)
-  return typeof cached === 'string' ? cachedContentFor(url, cached) : null
+  return typeof cached === 'string'
+    ? cachedContentFor(url, cached, idPrefix)
+    : null
 }
 
 export function GithubFallback({
@@ -207,14 +219,18 @@ export function GithubFallback({
 }: GithubFallbackProps) {
   const variant = mono ? 'mono' : 'full'
   const { baseUrl, branch } = useIconConfig()
+  // Per-instance prefix for the SVG's resource ids: without it two mounts
+  // of the same fetched SVG collide on document-global url(#...) lookups.
+  // The sanitize strips React's useId delimiters, unsafe in CSS url() tokens.
+  const idPrefix = useId().replace(/[^a-zA-Z0-9_-]/g, '')
   // Resolve the asset type once per render (pure, cheap-ish lookup) and
   // thread it through both the cache-read and the fetch path so core
   // `resolve` runs at most once.
   const type = inferType(id, iconType)
   const requestKey = `${baseUrl}|${branch}|${iconType ?? 'auto'}|${id}|${variant}`
   const cachedContent = useMemo(
-    () => getCachedContent(baseUrl, branch, type, id, variant),
-    [baseUrl, branch, type, id, variant],
+    () => getCachedContent(baseUrl, branch, type, id, variant, idPrefix),
+    [baseUrl, branch, type, id, variant, idPrefix],
   )
   const [state, setState] = useState<FallbackState>(() => ({
     key: requestKey,
@@ -241,10 +257,11 @@ export function GithubFallback({
         return
       }
       // Convert once and memoise on the cache entry so other mounts of the
-      // same URL reuse it. A null conversion (unparseable text) is
-      // negative-cached so other mounts don't re-attempt the parse;
-      // transient network misses never reach here (resolved is null).
-      const content = cachedContentFor(resolved.url, resolved.text)
+      // same URL reuse it (id-free SVGs only — see cachedContentFor). A null
+      // conversion (unparseable text) is negative-cached so other mounts
+      // don't re-attempt the parse; transient network misses never reach
+      // here (resolved is null).
+      const content = cachedContentFor(resolved.url, resolved.text, idPrefix)
       if (!content) cacheSet(resolved.url, null)
       setState({
         key: requestKey,
@@ -262,6 +279,7 @@ export function GithubFallback({
     currentState.failed,
     currentState.content,
     id,
+    idPrefix,
     requestKey,
     type,
     variant,

packages/react/src/svg-to-react.ts

@@ -6,22 +6,68 @@ import { styleStringToObject, toReactAttributeName } from './svg-attributes'
 export interface SvgReactContent {
   node: ReactNode
   viewBox: string
+  /**
+   * Whether the SVG defines resource ids (clipPaths, gradients, ...). Id
+   * lookups for url(#...) are document-global, so converted content with
+   * ids is only correct for the ONE instance whose prefix it carries —
+   * callers must not share it across mounts (see github-fallback.tsx).
+   */
+  hasIds: boolean
 }
 
-function svgNodeToReact(node: ChildNode, key: string): ReactNode {
+/**
+ * Rewrite fragment references to ids defined in this SVG. Handles
+ * `url(#x)` anywhere in a value and whole-value `#x` (href / xlink:href).
+ * References to ids the SVG does not define are left alone.
+ */
+function prefixRefs(value: string, ids: Set<string>, prefix: string): string {
+  let out = value
+  for (const id of ids) {
+    const ref = `url(#${id})`
+    if (out.includes(ref)) out = out.split(ref).join(`url(#${prefix}${id})`)
+  }
+  if (out.startsWith('#') && ids.has(out.slice(1))) {
+    out = `#${prefix}${out.slice(1)}`
+  }
+  return out
+}
+
+function svgNodeToReact(
+  node: ChildNode,
+  key: string,
+  ids: Set<string>,
+  idPrefix: string,
+): ReactNode {
   if (node.nodeType === Node.TEXT_NODE) return node.textContent
   if (node.nodeType !== Node.ELEMENT_NODE) return null
 
   const element = node as Element
   const props: Record<string, unknown> = { key }
   for (const attr of Array.from(element.attributes)) {
     const attrName = toReactAttributeName(attr.name)
-    props[attrName] =
-      attr.name === 'style' ? styleStringToObject(attr.value) : attr.value
+    if (attr.name === 'style') {
+      const style = styleStringToObject(attr.value)
+      if (ids.size > 0) {
+        for (const [prop, value] of Object.entries(style)) {
+          if (typeof value === 'string' && value.includes('url(#')) {
+            style[prop] = prefixRefs(value, ids, idPrefix)
+          }
+        }
+      }
+      props[attrName] = style
+    } else if (attrName === 'id' && ids.has(attr.value)) {
+      props[attrName] = `${idPrefix}${attr.value}`
+    } else if (ids.size > 0 && /[#(]/.test(attr.value)) {
+      props[attrName] = prefixRefs(attr.value, ids, idPrefix)
+    } else {
+      props[attrName] = attr.value
+    }
   }
 
   const children = Array.from(element.childNodes)
-    .map((child, index) => svgNodeToReact(child, `${key}-${index}`))
+    .map((child, index) =>
+      svgNodeToReact(child, `${key}-${index}`, ids, idPrefix),
+    )
     .filter((child) => child !== null)
 
   return createElement(element.tagName, props, ...children)
@@ -33,15 +79,27 @@ function svgNodeToReact(node: ChildNode, key: string): ReactNode {
  * root's viewBox; the caller renders its own `<svg>` wrapper so it controls
  * sizing and accessibility attributes.
  *
+ * `idPrefix` namespaces every id the SVG defines (and the url(#)/href refs
+ * to them) so multiple mounted instances of the same SVG don't collide on
+ * document-global id lookups. Pass a per-instance prefix (e.g. derived from
+ * useId()) when the same SVG text can be mounted more than once.
+ *
  * Returns null when the input is not a parseable `<svg>` document (or when
  * no DOM is available, e.g. during SSR).
  */
-export function svgTextToReact(svgText: string): SvgReactContent | null {
+export function svgTextToReact(
+  svgText: string,
+  idPrefix = '',
+): SvgReactContent | null {
   const root = sanitizeSvgRoot(svgText)
   if (!root) return null
   const viewBox = root.getAttribute('viewBox') ?? '0 0 32 32'
+  const ids = new Set<string>()
+  for (const el of Array.from(root.querySelectorAll('[id]'))) {
+    ids.add(el.id)
+  }
   const node = Array.from(root.childNodes)
-    .map((child, index) => svgNodeToReact(child, String(index)))
+    .map((child, index) => svgNodeToReact(child, String(index), ids, idPrefix))
     .filter((child) => child !== null)
-  return { node, viewBox }
+  return { node, viewBox, hasIds: ids.size > 0 }
 }

scripts/generate.ts

@@ -16,6 +16,7 @@ import {
   extractViewBox,
   createComponentName,
   capitalize,
+  parameterizeIds,
 } from './svg-to-jsx'
 import {
   loadCatalogue,
@@ -274,7 +275,14 @@ function displayNameLines(componentName: string): string[] {
 // branch the combined component used to inline.
 function generateVariantFile(spec: ComponentSpec, variant: Variant): string {
   const svg = variant === 'mono' ? spec.monoSvg : spec.fullSvg
-  const inner = extractSvgInner(svgToJsx(svg))
+  // SVG resource ids (clipPaths, gradients) must be unique per mounted
+  // INSTANCE, not just per asset: url(#...) resolves document-wide to the
+  // first match, so a second copy of the same icon — e.g. one in a hidden
+  // drawer — hijacks the refs of every visible copy and blanks it. Icons
+  // that define ids get a useId()-derived prefix baked into ids and refs.
+  const { jsx: inner, idCount } = parameterizeIds(
+    extractSvgInner(svgToJsx(svg)),
+  )
   const viewBox = extractViewBox(svg) || '0 0 32 32'
   // Suffixed so React DevTools distinguishes the variant actually mounted
   // (the combined component renders one of the two).
@@ -293,23 +301,44 @@ function generateVariantFile(spec: ComponentSpec, variant: Variant): string {
     fillAttr = /<svg[^>]*fill="none"/.test(svg) ? ' fill="none"' : ''
   }
 
+  // `mono` is consumed by the combined component; here it is discarded so
+  // it never reaches the DOM <svg>. Icons that define resource ids get a
+  // per-instance prefix: useId() is unique per mount and SSR-stable, and the
+  // sanitize strips React's delimiter characters (`:` / `«»`), which are not
+  // safe inside an unquoted CSS url() token.
+  const bodyLines =
+    idCount > 0
+      ? [
+          '  ({ mono: _mono, size = 32, ...props }, ref) => {',
+          "    const uid = useId().replace(/[^a-zA-Z0-9_-]/g, '')",
+          '    return (',
+          `      <svg ref={ref} width={size} height={size} viewBox="${viewBox}"${fillAttr} role="img" aria-label="${spec.ariaLabel}" {...props}>`,
+          `        ${inner}`,
+          '      </svg>',
+          '    )',
+          '  },',
+        ]
+      : [
+          '  ({ mono: _mono, size = 32, ...props }, ref) => (',
+          `    <svg ref={ref} width={size} height={size} viewBox="${viewBox}"${fillAttr} role="img" aria-label="${spec.ariaLabel}" {...props}>`,
+          `      ${inner}`,
+          '    </svg>',
+          '  ),',
+        ]
+
   return [
     '// Auto-generated by scripts/generate.ts - DO NOT EDIT',
-    "import { forwardRef } from 'react'",
+    idCount > 0
+      ? "import { forwardRef, useId } from 'react'"
+      : "import { forwardRef } from 'react'",
     `import type { ${spec.propsType} } from '../../icon-props'`,
     '',
     // PURE annotation: forwardRef() is opaque to bundlers, and chunk-mates
     // inside one shared module are only droppable when the init is provably
     // side-effect-free. Together with the NODE_ENV displayName gate this is
     // what makes single-icon imports tree-shake (see tree-shake.mjs fixture).
     `const ${variantName} = /* @__PURE__ */ forwardRef<SVGSVGElement, ${spec.propsType}>(`,
-    // `mono` is consumed by the combined component; here it is discarded so
-    // it never reaches the DOM <svg>.
-    '  ({ mono: _mono, size = 32, ...props }, ref) => (',
-    `    <svg ref={ref} width={size} height={size} viewBox="${viewBox}"${fillAttr} role="img" aria-label="${spec.ariaLabel}" {...props}>`,
-    `      ${inner}`,
-    '    </svg>',
-    '  ),',
+    ...bodyLines,
     ')',
     ...displayNameLines(variantName),
     '',