fix(hub-ui): retry transient icon requests

Author: dvcolomban

packages/hub-ui/package.json

@@ -66,6 +66,7 @@
     "dompurify": "catalog:frontend",
     "fuse.js": "catalog:frontend",
     "iframe-pane": "catalog:frontend",
+    "jsdom": "catalog:testing",
     "storybook": "catalog:storybook",
     "tsdown": "catalog:build",
     "tsx": "catalog:build",

packages/hub-ui/src/client/components/icons/IconifyIcon.test.ts

@@ -0,0 +1,53 @@
+// @vitest-environment jsdom
+import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'
+import { createApp, h, nextTick, ref } from 'vue'
+import IconifyIcon from './IconifyIcon.vue'
+
+const pending = vi.hoisted(() => new Map<string, (svg: string) => void>())
+vi.mock('../../utils/iconify', () => ({
+  getIconifySvg: vi.fn((_collection: string, icon: string) => new Promise<string>((resolve) => {
+    pending.set(icon, resolve)
+  })),
+}))
+
+const applications: ReturnType<typeof createApp>[] = []
+beforeEach(() => pending.clear())
+afterEach(() => {
+  applications.splice(0).forEach(application => application.unmount())
+  document.body.replaceChildren()
+})
+
+describe('iconifyIcon async rendering', () => {
+  it('renders a resolved icon on the same mount', async () => {
+    expect.assertions(2)
+    const container = document.createElement('div')
+    document.body.append(container)
+    const application = createApp(IconifyIcon, { icon: 'ph:first' })
+    applications.push(application)
+    application.mount(container)
+    expect(container.querySelector('svg')).toBeNull()
+    pending.get('first')!('<svg data-icon="first"/>')
+    await Promise.resolve()
+    await nextTick()
+    expect(container.querySelector('svg')?.getAttribute('data-icon')).toBe('first')
+  })
+
+  it('ignores an old response after the icon changes', async () => {
+    expect.assertions(2)
+    const icon = ref('ph:first')
+    const container = document.createElement('div')
+    const application = createApp({ render: () => h(IconifyIcon, { icon: icon.value }) })
+    applications.push(application)
+    application.mount(container)
+    icon.value = 'ph:second'
+    await nextTick()
+    pending.get('second')!('<svg data-icon="second"/>')
+    await Promise.resolve()
+    await nextTick()
+    expect(container.querySelector('svg')?.getAttribute('data-icon')).toBe('second')
+    pending.get('first')!('<svg data-icon="first"/>')
+    await Promise.resolve()
+    await nextTick()
+    expect(container.querySelector('svg')?.getAttribute('data-icon')).toBe('second')
+  })
+})

packages/hub-ui/src/client/components/icons/IconifyIcon.vue

@@ -20,18 +20,26 @@ const iconifyParsed = computed(() => {
 })
 
 const iconifyLoaded = ref<string | undefined>(undefined)
-watchEffect(async () => {
+watchEffect(async (onCleanup) => {
+  let active = true
+  onCleanup(() => {
+    active = false
+  })
+  iconifyLoaded.value = undefined
   if (!iconifyParsed.value) {
     iconifyLoaded.value = undefined
     return
   }
   try {
-    iconifyLoaded.value = await getIconifySvg(iconifyParsed.value.collection, iconifyParsed.value.icon)
+    const svg = await getIconifySvg(iconifyParsed.value.collection, iconifyParsed.value.icon)
+    if (active)
+      iconifyLoaded.value = svg
   }
   catch {
     // A failed icon fetch (offline / flaky CDN) should degrade to a blank icon,
     // not throw out of the async effect and crash the surrounding panel.
-    iconifyLoaded.value = undefined
+    if (active)
+      iconifyLoaded.value = undefined
   }
 })
 </script>

packages/hub-ui/src/client/utils/iconify.test.ts

@@ -0,0 +1,151 @@
+// @vitest-environment jsdom
+import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'
+
+const svg = '<svg xmlns="http://www.w3.org/2000/svg" viewBox="0 0 24 24"><path fill="currentColor" d="M0 0h24v24H0z"/></svg>'
+
+beforeEach(() => {
+  vi.resetModules()
+  vi.useFakeTimers()
+})
+
+afterEach(() => {
+  vi.unstubAllGlobals()
+  vi.restoreAllMocks()
+  vi.useRealTimers()
+})
+
+describe('remote icons', () => {
+  it('recovers the actual mounted component after a transient failure', async () => {
+    expect.assertions(3)
+    const fetchMock = vi.fn().mockRejectedValueOnce(new TypeError('offline')).mockResolvedValue(new Response(svg))
+    vi.stubGlobal('fetch', fetchMock)
+    const { createApp, nextTick } = await import('vue')
+    const { default: IconifyIcon } = await import('../components/icons/IconifyIcon.vue')
+    const container = document.createElement('div')
+    const application = createApp(IconifyIcon, { icon: 'ph:robot-duotone' })
+    application.mount(container)
+    try {
+      expect(container.querySelector('svg')).toBeNull()
+      await vi.advanceTimersByTimeAsync(500)
+      await nextTick()
+      expect(container.querySelector('svg')).not.toBeNull()
+      expect(fetchMock).toHaveBeenCalledTimes(2)
+    }
+    finally {
+      application.unmount()
+    }
+  })
+
+  it('lets a shared request finish after one consumer unmounts', async () => {
+    expect.assertions(3)
+    const fetchMock = vi.fn().mockRejectedValueOnce(new TypeError('offline')).mockResolvedValue(new Response(svg))
+    vi.stubGlobal('fetch', fetchMock)
+    const { createApp, nextTick } = await import('vue')
+    const { default: IconifyIcon } = await import('../components/icons/IconifyIcon.vue')
+    const firstContainer = document.createElement('div')
+    const secondContainer = document.createElement('div')
+    const first = createApp(IconifyIcon, { icon: 'ph:robot-duotone' })
+    const second = createApp(IconifyIcon, { icon: 'ph:robot-duotone' })
+    first.mount(firstContainer)
+    second.mount(secondContainer)
+    first.unmount()
+    try {
+      await vi.advanceTimersByTimeAsync(500)
+      await nextTick()
+      expect(firstContainer.innerHTML).toBe('')
+      expect(secondContainer.querySelector('svg')).not.toBeNull()
+      expect(fetchMock).toHaveBeenCalledTimes(2)
+    }
+    finally {
+      second.unmount()
+    }
+  })
+
+  it('shares retries and caches the successful SVG', async () => {
+    expect.assertions(6)
+    const fetchMock = vi.fn().mockRejectedValueOnce(new TypeError('offline')).mockResolvedValue(new Response(svg))
+    vi.stubGlobal('fetch', fetchMock)
+    const { getIconifySvg } = await import('./iconify')
+    const first = getIconifySvg('ph', 'robot-duotone')
+    const second = getIconifySvg('ph', 'robot-duotone')
+    await vi.advanceTimersByTimeAsync(499)
+    expect(fetchMock).toHaveBeenCalledTimes(1)
+    await vi.advanceTimersByTimeAsync(1)
+    expect(await first).toContain('<svg')
+    expect(await second).toBe(await first)
+    expect(fetchMock).toHaveBeenCalledTimes(2)
+    expect(await getIconifySvg('ph', 'robot-duotone')).toBe(await first)
+    expect(fetchMock).toHaveBeenCalledTimes(2)
+  })
+
+  it.each([408, 429, 500, 503])('retries HTTP %i', async (status) => {
+    expect.assertions(2)
+    const fetchMock = vi.fn().mockResolvedValueOnce(new Response('', { status })).mockResolvedValue(new Response(svg))
+    vi.stubGlobal('fetch', fetchMock)
+    const { getIconifySvg } = await import('./iconify')
+    const result = getIconifySvg('ph', 'robot-duotone')
+    await vi.advanceTimersByTimeAsync(500)
+    expect(await result).toContain('<svg')
+    expect(fetchMock).toHaveBeenCalledTimes(2)
+  })
+
+  it('exhausts three attempts and allows a later request', async () => {
+    expect.assertions(5)
+    const fetchMock = vi.fn().mockRejectedValue(new TypeError('offline'))
+    vi.stubGlobal('fetch', fetchMock)
+    const { getIconifySvg } = await import('./iconify')
+    const result = expect(getIconifySvg('ph', 'robot-duotone')).rejects.toThrow('offline')
+    await vi.advanceTimersByTimeAsync(500)
+    expect(fetchMock).toHaveBeenCalledTimes(2)
+    await vi.advanceTimersByTimeAsync(1499)
+    expect(fetchMock).toHaveBeenCalledTimes(2)
+    await vi.advanceTimersByTimeAsync(1)
+    await result
+    expect(fetchMock).toHaveBeenCalledTimes(3)
+    fetchMock.mockResolvedValue(new Response(svg))
+    expect(await getIconifySvg('ph', 'robot-duotone')).toContain('<svg')
+  })
+
+  it.each([400, 403, 404])('does not retry or cache HTTP %i', async (status) => {
+    expect.assertions(3)
+    const fetchMock = vi.fn().mockResolvedValueOnce(new Response(svg, { status })).mockResolvedValue(new Response(svg))
+    vi.stubGlobal('fetch', fetchMock)
+    const { getIconifySvg } = await import('./iconify')
+    await expect(getIconifySvg('ph', 'robot-duotone')).rejects.toThrow()
+    expect(fetchMock).toHaveBeenCalledTimes(1)
+    expect(await getIconifySvg('ph', 'robot-duotone')).toContain('<svg')
+  })
+
+  it.each(['', '<html>Unavailable</html>', '<svg xmlns="http://www.w3.org/2000/svg"><script>alert(1)</script></svg>'])('rejects invalid SVG without caching it', async (body) => {
+    expect.assertions(3)
+    const fetchMock = vi.fn().mockResolvedValueOnce(new Response(body)).mockResolvedValue(new Response(svg))
+    vi.stubGlobal('fetch', fetchMock)
+    const { getIconifySvg } = await import('./iconify')
+    await expect(getIconifySvg('ph', 'robot-duotone')).rejects.toThrow()
+    expect(fetchMock).toHaveBeenCalledTimes(1)
+    expect(await getIconifySvg('ph', 'robot-duotone')).toContain('<svg')
+  })
+
+  it('bounds each attempt to ten seconds', async () => {
+    expect.assertions(3)
+    const timeout = vi.spyOn(AbortSignal, 'timeout')
+    const fetchMock = vi.fn().mockRejectedValueOnce(new DOMException('timeout', 'TimeoutError')).mockResolvedValue(new Response(svg))
+    vi.stubGlobal('fetch', fetchMock)
+    const { getIconifySvg } = await import('./iconify')
+    const result = getIconifySvg('ph', 'robot-duotone')
+    await vi.advanceTimersByTimeAsync(500)
+    expect(await result).toContain('<svg')
+    expect(timeout).toHaveBeenCalledTimes(2)
+    expect(timeout).toHaveBeenNthCalledWith(2, 10_000)
+  })
+
+  it('sanitizes executable content while preserving currentColor', async () => {
+    expect.assertions(3)
+    vi.stubGlobal('fetch', vi.fn().mockResolvedValue(new Response(svg.replace('<path ', '<script>alert(1)</script><path onclick="alert(1)" '))))
+    const { getIconifySvg } = await import('./iconify')
+    const result = await getIconifySvg('ph', 'robot-duotone')
+    expect(result).toContain('currentColor')
+    expect(result).not.toContain('<script')
+    expect(result).not.toContain('onclick')
+  })
+})

packages/hub-ui/src/client/utils/iconify.ts

@@ -4,12 +4,51 @@ const getIconifySvgMap = new Map<string, Promise<string> | string>()
 
 const purify = createDOMPurify()
 
+function sanitizeSvg(svg: string): string {
+  const sanitized = purify.sanitize(svg)
+  const document = new DOMParser().parseFromString(sanitized, 'image/svg+xml')
+  if (document.documentElement.localName !== 'svg'
+    || document.querySelector('parsererror')
+    || !document.querySelector('path, rect, circle, ellipse, polygon, polyline, line, text, use, image')) {
+    throw new Error('Invalid icon SVG')
+  }
+  return sanitized
+}
+
+async function fetchIconSvg(url: string): Promise<string> {
+  const delays = [500, 1500]
+  for (let attempt = 0; ; attempt++) {
+    let response: Response
+    let body: string | undefined
+    try {
+      response = await fetch(url, { signal: AbortSignal.timeout(10_000) })
+      if (response.ok)
+        body = await response.text()
+    }
+    catch (error) {
+      if (attempt >= delays.length)
+        throw error
+      await new Promise(resolve => setTimeout(resolve, delays[attempt]))
+      continue
+    }
+
+    if (body !== undefined)
+      return sanitizeSvg(body)
+
+    const retryable = response.status === 408 || response.status === 429 || response.status >= 500
+    if (!retryable || attempt >= delays.length)
+      throw new Error(`Icon request failed: HTTP ${response.status}`)
+    await new Promise(resolve => setTimeout(resolve, delays[attempt]))
+  }
+}
+
 export async function getIconifySvg(collection: string, icon: string) {
   const id = `${collection}:${icon}`
   if (getIconifySvgMap.has(id)) {
     return getIconifySvgMap.get(id)!
   }
-  const promise = _get()
+  const url = `https://api.iconify.design/${collection}/${icon}.svg?color=currentColor&width=100%`
+  const promise = fetchIconSvg(url)
     .then((svg) => {
       getIconifySvgMap.set(id, svg)
       return svg
@@ -21,13 +60,4 @@ export async function getIconifySvg(collection: string, icon: string) {
     })
   getIconifySvgMap.set(id, promise)
   return promise
-
-  async function _get() {
-    const url = `https://api.iconify.design/${collection}/${icon}.svg?color=currentColor&width=100%`
-    // Bound the request so a stalled connection (offline / flaky CDN / firewall
-    // black-holing the host) rejects instead of hanging forever; the caller
-    // already degrades a rejected fetch to a blank icon.
-    const svg = await fetch(url, { signal: AbortSignal.timeout(10_000) }).then(res => res.text())
-    return purify.sanitize(svg)
-  }
 }

packages/hub-ui/vitest.config.ts

@@ -0,0 +1,9 @@
+import Vue from '@vitejs/plugin-vue'
+import { defineProject } from 'vitest/config'
+import { alias } from '../../alias'
+
+export default defineProject({
+  plugins: [Vue()],
+  resolve: { alias },
+  test: { name: 'hub-ui' },
+})