fix(webui): resilient dashboard polling (SF7)

useDashboardStore.refresh() used Promise.all across overview/usage/ targets: one route rejecting threw before any of the three state fields were assigned, so a single flaky GET /admin/api/targets blanked the already-working Providers and Usage views too. Switched to Promise.allSettled with per-section assignment: a fulfilled result updates its own field, a rejected one leaves the PREVIOUS value in place rather than nulling it. lastUpdated still advances on a partial success (some fresh data did arrive); error names which section(s) failed. A 401/403 on any section still defers to the auth store exactly as before (AdminApiError check unchanged). Added dashboard.spec.ts — the first Pinia store under vitest in this package (a store needs no DOM, so it fits the existing 'node' environment without pulling in jsdom); vite.config.ts's own test-scope comment updated to say so.
2026-10-12 04:54:45 +00:00 · 2026-08-21 19:16:32 +01:00
parent 8afb903227
commit a25ebf0567
3 changed files with 187 additions and 22 deletions
@@ -0,0 +1,137 @@
+// dashboard.spec.ts covers useDashboardStore.refresh's per-section
+// resilience (review round 2, v0.21 fix, SF7): GET /admin/api/overview,
+// /usage, and /targets are fetched via Promise.allSettled and assigned
+// independently — one endpoint failing must never blank the OTHER two,
+// already-populated sections, and must never wipe an already-successful
+// section's PREVIOUS value on a later failed poll.
+//
+// This is the first Pinia store under vitest in this package (vite.config.ts's
+// own `test` block comment — "no snapshot/DOM/component testing this
+// round" — predates this file). A Pinia store needs no DOM: it is plain
+// reactive state and actions, well within the 'node' test environment
+// already configured; adminFetch (lib/api.ts) is mocked entirely, so no
+// real network or sessionStorage access ever happens.
+
+import { createPinia, setActivePinia } from 'pinia'
+import { beforeEach, describe, expect, it, vi } from 'vitest'
+
+vi.mock('@/lib/api', async () => {
+  const actual = await vi.importActual<typeof import('@/lib/api')>('@/lib/api')
+  return { ...actual, adminFetch: vi.fn() }
+})
+
+import { adminFetch, AdminApiError } from '@/lib/api'
+import { useAuthStore } from '@/stores/auth'
+import { useDashboardStore } from '@/stores/dashboard'
+import type { AdminOverviewResponse, AdminTargetsResponse, AdminUsageResponse } from '@/types/api'
+
+const mockedAdminFetch = vi.mocked(adminFetch)
+
+const emptyOverview: AdminOverviewResponse = {
+  version: '1',
+  providers: [],
+  groups: [],
+  aliases: [],
+  redis: { configured: false, lastErrAt: '0001-01-01T00:00:00Z' },
+  cache: { enabled: false },
+  retry: { enabled: false },
+}
+const emptyUsage: AdminUsageResponse = {
+  users: [],
+  groups: [],
+  total: {
+    kind: 'total',
+    id: 'all',
+    requestsPerMinute: 0,
+    requestsPerDay: 0,
+    tokensInPerDay: 0,
+    tokensOutPerDay: 0,
+    tokensInPerMonth: 0,
+    tokensOutPerMonth: 0,
+    costPerDayMicroUsd: 0,
+    costPerMonthMicroUsd: 0,
+  },
+}
+const emptyTargets: AdminTargetsResponse = { mcpServers: [], agents: [] }
+
+/** mockRoutes wires adminFetch's mock to answer each of the three routes independently — a route with no override answers its own empty fixture; an override that throws simulates that one route failing. */
+function mockRoutes(routes: {
+  overview?: () => AdminOverviewResponse
+  usage?: () => AdminUsageResponse
+  targets?: () => AdminTargetsResponse
+}): void {
+  mockedAdminFetch.mockImplementation((async (path: string) => {
+    if (path === '/admin/api/overview') return routes.overview ? routes.overview() : emptyOverview
+    if (path === '/admin/api/usage') return routes.usage ? routes.usage() : emptyUsage
+    if (path === '/admin/api/targets') return routes.targets ? routes.targets() : emptyTargets
+    throw new Error(`unexpected path ${path}`)
+  }) as typeof adminFetch)
+}
+
+describe('useDashboardStore.refresh', () => {
+  beforeEach(() => {
+    setActivePinia(createPinia())
+    mockedAdminFetch.mockReset()
+    useAuthStore().submit('test-admin-key')
+  })
+
+  it('assigns all three sections and clears error on full success', async () => {
+    mockRoutes({})
+    const dashboard = useDashboardStore()
+    await dashboard.refresh()
+
+    expect(dashboard.overview).not.toBeNull()
+    expect(dashboard.usage).not.toBeNull()
+    expect(dashboard.targets).not.toBeNull()
+    expect(dashboard.error).toBe('')
+    expect(dashboard.lastUpdated).not.toBeNull()
+  })
+
+  it('keeps overview and usage populated when targets alone fails', async () => {
+    mockRoutes({
+      targets: () => {
+        throw new Error('targets boom')
+      },
+    })
+    const dashboard = useDashboardStore()
+    await dashboard.refresh()
+
+    expect(dashboard.overview).not.toBeNull()
+    expect(dashboard.usage).not.toBeNull()
+    expect(dashboard.targets).toBeNull() // never successfully fetched yet — nothing to keep
+    expect(dashboard.error).toContain('targets')
+    expect(dashboard.lastUpdated).not.toBeNull() // a partial success still advances it
+  })
+
+  it('preserves a previously-successful section instead of blanking it on a later failure', async () => {
+    const dashboard = useDashboardStore()
+
+    mockRoutes({})
+    await dashboard.refresh()
+    const seededTargets = dashboard.targets
+    expect(seededTargets).not.toBeNull()
+
+    mockRoutes({
+      targets: () => {
+        throw new Error('targets boom again')
+      },
+    })
+    await dashboard.refresh()
+
+    expect(dashboard.targets).toBe(seededTargets) // same object — never reassigned to null
+    expect(dashboard.overview).not.toBeNull()
+    expect(dashboard.usage).not.toBeNull()
+  })
+
+  it('does not surface an error when a section rejects with 401 (the auth store owns that case)', async () => {
+    mockRoutes({
+      targets: () => {
+        throw new AdminApiError('unauthorized', 401)
+      },
+    })
+    const dashboard = useDashboardStore()
+    await dashboard.refresh()
+
+    expect(dashboard.error).toBe('')
+  })
+})
@@ -30,25 +30,49 @@ export const useDashboardStore = defineStore('dashboard', {
    async refresh(): Promise<void> {
      const auth = useAuthStore()
      if (!auth.isAuthenticated) return
-      try {
-        const [overview, usage, targets] = await Promise.all([
-          adminFetch<AdminOverviewResponse>('/admin/api/overview'),
-          adminFetch<AdminUsageResponse>('/admin/api/usage'),
-          adminFetch<AdminTargetsResponse>('/admin/api/targets'),
-        ])
-        this.overview = overview
-        this.usage = usage
-        this.targets = targets
+
+      const [overviewResult, usageResult, targetsResult] = await Promise.allSettled([
+        adminFetch<AdminOverviewResponse>('/admin/api/overview'),
+        adminFetch<AdminUsageResponse>('/admin/api/usage'),
+        adminFetch<AdminTargetsResponse>('/admin/api/targets'),
+      ])
+
+      // Each section is assigned independently (review round 2, v0.21
+      // fix): one endpoint failing (e.g. GET /admin/api/targets) must
+      // never blank the OTHER two, already-populated sections. The
+      // earlier Promise.all threw on the FIRST rejection and updated
+      // nothing at all — a single flaky route degraded every view in the
+      // dashboard, not just its own.
+      if (overviewResult.status === 'fulfilled') this.overview = overviewResult.value
+      if (usageResult.status === 'fulfilled') this.usage = usageResult.value
+      if (targetsResult.status === 'fulfilled') this.targets = targetsResult.value
+
+      const failures: { label: string; reason: unknown }[] = []
+      if (overviewResult.status === 'rejected') failures.push({ label: 'overview', reason: overviewResult.reason })
+      if (usageResult.status === 'rejected') failures.push({ label: 'usage', reason: usageResult.reason })
+      if (targetsResult.status === 'rejected') failures.push({ label: 'targets', reason: targetsResult.reason })
+
+      if (failures.length === 0) {
        this.lastUpdated = new Date()
        this.error = ''
-      } catch (err) {
-        // A 401/403 already rejected the key (lib/api.ts) — the AuthGate
-        // takes over the view, nothing left to report here.
-        if (err instanceof AdminApiError && (err.status === 401 || err.status === 403)) {
-          return
-        }
-        this.error = err instanceof Error ? err.message : String(err)
+        return
      }
+
+      // A 401/403 on ANY section already rejected the key (lib/api.ts) —
+      // the AuthGate takes over the whole view in that case, matching the
+      // pre-existing single-Promise.all behavior; nothing left to report.
+      const authRejected = failures.some(
+        ({ reason }) => reason instanceof AdminApiError && (reason.status === 401 || reason.status === 403),
+      )
+      if (authRejected) return
+
+      // A partial failure still advances lastUpdated: at least one
+      // section genuinely has fresh data, even though `error` (below)
+      // still surfaces that something is degraded.
+      if (failures.length < 3) this.lastUpdated = new Date()
+      this.error = failures
+        .map(({ label, reason }) => `${label}: ${reason instanceof Error ? reason.message : String(reason)}`)
+        .join('; ')
    },
    /** startPolling is idempotent and safe to call before a key is stored — refresh() no-ops until then. */
    startPolling(): void {
@@ -33,12 +33,16 @@ export default defineConfig({
    modulePreload: { polyfill: false },
  },
  test: {
-    // Deliberately narrow: only lib/'s pure modules (search-expand.ts,
-    // usage-search.ts) are under vitest right now (webui's test infra
-    // starts here, on purpose — no snapshot/DOM/component testing this
-    // round). 'node' is enough since neither module touches browser
-    // globals; jsdom is not pulled in as a dependency for modules that
-    // don't need it.
+    // Deliberately narrow: lib/'s pure modules (search-expand.ts,
+    // usage-search.ts) plus, since review round 2 (v0.21), stores/
+    // dashboard.ts's own refresh() action — a Pinia store is plain
+    // reactive state and actions, no DOM or component mount involved, so
+    // it fits this same environment. Still no snapshot/DOM/component
+    // (.vue mount) testing. 'node' is enough since nothing under test
+    // touches a real browser global — dashboard.spec.ts mocks lib/api.ts
+    // entirely, so even sessionStorage access (auth.ts) never actually
+    // runs; jsdom is not pulled in as a dependency for modules that don't
+    // need it.
    environment: 'node',
    include: ['src/**/*.spec.ts'],
  },