From a25ebf0567c02249405354d54a5049c9c50f3d07 Mon Sep 17 00:00:00 2001 From: Lukasz Raczylo Date: Fri, 21 Aug 2026 19:16:32 +0100 Subject: [PATCH] fix(webui): resilient dashboard polling (SF7) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- webui/src/stores/dashboard.spec.ts | 137 +++++++++++++++++++++++++++++ webui/src/stores/dashboard.ts | 56 ++++++++---- webui/vite.config.ts | 16 ++-- 3 files changed, 187 insertions(+), 22 deletions(-) create mode 100644 webui/src/stores/dashboard.spec.ts diff --git a/webui/src/stores/dashboard.spec.ts b/webui/src/stores/dashboard.spec.ts new file mode 100644 index 0000000..302f731 --- /dev/null +++ b/webui/src/stores/dashboard.spec.ts @@ -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('@/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('') + }) +}) diff --git a/webui/src/stores/dashboard.ts b/webui/src/stores/dashboard.ts index bb87606..dbbc9a4 100644 --- a/webui/src/stores/dashboard.ts +++ b/webui/src/stores/dashboard.ts @@ -30,25 +30,49 @@ export const useDashboardStore = defineStore('dashboard', { async refresh(): Promise { const auth = useAuthStore() if (!auth.isAuthenticated) return - try { - const [overview, usage, targets] = await Promise.all([ - adminFetch('/admin/api/overview'), - adminFetch('/admin/api/usage'), - adminFetch('/admin/api/targets'), - ]) - this.overview = overview - this.usage = usage - this.targets = targets + + const [overviewResult, usageResult, targetsResult] = await Promise.allSettled([ + adminFetch('/admin/api/overview'), + adminFetch('/admin/api/usage'), + adminFetch('/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 { diff --git a/webui/vite.config.ts b/webui/vite.config.ts index 3d3bc89..c4b3a59 100644 --- a/webui/vite.config.ts +++ b/webui/vite.config.ts @@ -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'], },