From a4a74ca9e9a0f7d7d8e731d927139ccba0a588ec Mon Sep 17 00:00:00 2001 From: kshitijk4poor Date: Wed, 24 Jun 2026 18:20:45 +0530 Subject: [PATCH] fix(desktop): use notify() with stable id for fallback notification MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit hermes-pr-review findings: - notifyError('runtime-not-ready', msg) misused the (error, fallback) API: the key became the notification body and the message became the title. Switch to notify({ id, kind, title, message }) which puts content in the right slots. - The stable id 'runtime-not-ready' deduplicates: notify() replaces by id, so repeated refreshOnboarding calls during an outage no longer stack up to 4 persistent error toasts. - Remove dead !state.manual guard from shouldPreserveConfiguredOnFallback: refreshOnboarding already short-circuits on manual before the helper. - Test: seed localStorage with '1' before asserting it survives (was testing the wrong invariant — null in, null out). - Test: use static import for spy instead of fragile await import. - Test: add negative case for requested=true + configured=true (should still downgrade — requested overrides preservation). --- apps/desktop/src/store/onboarding.test.ts | 47 +++++++++++++++++++---- apps/desktop/src/store/onboarding.ts | 15 +++++--- 2 files changed, 48 insertions(+), 14 deletions(-) diff --git a/apps/desktop/src/store/onboarding.test.ts b/apps/desktop/src/store/onboarding.test.ts index 0184ac5b64b..806dafc7bf6 100644 --- a/apps/desktop/src/store/onboarding.test.ts +++ b/apps/desktop/src/store/onboarding.test.ts @@ -2,6 +2,8 @@ import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' import type { OAuthProvider } from '@/types/hermes' +import * as notifications from '@/store/notifications' + import { $desktopOnboarding, type DesktopOnboardingState, @@ -136,6 +138,8 @@ describe('refreshOnboarding', () => { }) installApiMock(api) + // Simulate a returning user: cache is set and store is configured. + window.localStorage.setItem('hermes-desktop-onboarded-v1', '1') $desktopOnboarding.set( baseState({ configured: true, @@ -151,14 +155,12 @@ describe('refreshOnboarding', () => { expect(api).not.toHaveBeenCalled() expect($desktopOnboarding.get().configured).toBe(true) expect($desktopOnboarding.get().reason).toBeNull() - expect(window.localStorage.getItem('hermes-desktop-onboarded-v1')).toBeNull() + // The cache must survive the refresh — proving we didn't downgrade. + expect(window.localStorage.getItem('hermes-desktop-onboarded-v1')).toBe('1') }) it('shows a non-blocking notification when preserving configured on fallback', async () => { - const notifyErrorSpy = vi.spyOn( - await import('@/store/notifications'), - 'notifyError' - ) + const notifySpy = vi.spyOn(notifications, 'notify') installApiMock(vi.fn()) $desktopOnboarding.set( @@ -172,13 +174,42 @@ describe('refreshOnboarding', () => { await refreshOnboarding(onboardingContext(fallbackTimeoutGateway())) - expect(notifyErrorSpy).toHaveBeenCalledWith( - 'runtime-not-ready', - expect.stringContaining('could not verify the running backend') + expect(notifySpy).toHaveBeenCalledWith( + expect.objectContaining({ + id: 'runtime-not-ready', + kind: 'error' + }) ) expect($desktopOnboarding.get().configured).toBe(true) }) + it('does not preserve configured when onboarding was explicitly requested', async () => { + const api = vi.fn(async ({ path }: { path: string }) => { + if (path === '/api/providers/oauth') { + return { providers: [provider('fresh')] } + } + + throw new Error(`unexpected api path: ${path}`) + }) + + installApiMock(api) + $desktopOnboarding.set( + baseState({ + configured: true, + providers: [provider('cached')], + reason: null, + requested: true + }) + ) + + const ready = await refreshOnboarding(onboardingContext(fallbackTimeoutGateway())) + + expect(ready).toBe(false) + // requested overrides preservation — should downgrade. + expect($desktopOnboarding.get().configured).toBe(false) + expect(api).toHaveBeenCalledTimes(1) + }) + it('still surfaces onboarding when fallback failure happens before configured state', async () => { const api = vi.fn(async ({ path }: { path: string }) => { if (path === '/api/providers/oauth') { diff --git a/apps/desktop/src/store/onboarding.ts b/apps/desktop/src/store/onboarding.ts index b5de18d64b5..33c32a1033b 100644 --- a/apps/desktop/src/store/onboarding.ts +++ b/apps/desktop/src/store/onboarding.ts @@ -195,7 +195,7 @@ function shouldPreserveConfiguredOnFallback( // A fallback result means both runtime probes were non-authoritative // (transport timeout/disconnect). Keep a previously verified configured // state instead of forcing the blocking onboarding overlay. - return runtime.source === 'fallback' && state.configured === true && !state.requested && !state.manual + return runtime.source === 'fallback' && state.configured === true && !state.requested } function notifyReady(provider: string) { @@ -528,11 +528,14 @@ export async function refreshOnboarding(ctx: OnboardingContext) { if (shouldPreserveConfiguredOnFallback(runtime, state)) { // Gateway probes timed out but the user was already configured — don't // downgrade to the blocking onboarding overlay. Surface a non-blocking - // notification so the user knows the backend wasn't verified. - notifyError( - 'runtime-not-ready', - 'Hermes Desktop could not verify the running backend on startup. Some features may be unavailable until the gateway is reachable.' - ) + // notification with a stable id so repeated calls during an outage dedup + // instead of stacking toasts. + notify({ + id: 'runtime-not-ready', + kind: 'error', + title: 'Runtime not ready', + message: 'Hermes Desktop could not verify the running backend on startup. Some features may be unavailable until the gateway is reachable.' + }) return false }