From f2d5351caf05f8b074cd6050e5cad28341afb6bd Mon Sep 17 00:00:00 2001 From: ousugo Date: Sat, 4 Jul 2026 21:14:45 +0800 Subject: [PATCH 1/3] fix(provider): move auto-enabled providers to top --- .../useProviderModelPullReconcile.test.ts | 8 ++ .../__tests__/usePullReconcileSubmit.test.ts | 9 +++ .../useProviderModelPullReconcile.ts | 13 +++- .../ModelList/usePullReconcileSubmit.ts | 6 +- .../useProviderAutoModelSync.test.tsx | 8 ++ .../useProviderConnectionCheck.test.tsx | 9 +++ .../useProviderAutoModelSync.ts | 23 +++++- .../useProviderConnectionCheck.ts | 22 +++++- .../__tests__/providerEnablement.test.ts | 77 ++++++++++++++++--- .../utils/providerEnablement.ts | 20 ++++- 10 files changed, 175 insertions(+), 20 deletions(-) diff --git a/src/renderer/pages/settings/ProviderSettings/ModelList/__tests__/useProviderModelPullReconcile.test.ts b/src/renderer/pages/settings/ProviderSettings/ModelList/__tests__/useProviderModelPullReconcile.test.ts index 659bfa625ef..0669a467eb1 100644 --- a/src/renderer/pages/settings/ProviderSettings/ModelList/__tests__/useProviderModelPullReconcile.test.ts +++ b/src/renderer/pages/settings/ProviderSettings/ModelList/__tests__/useProviderModelPullReconcile.test.ts @@ -10,6 +10,11 @@ const enableProviderWhenModelsAvailableMock = vi.fn() const updateProviderMock = vi.fn() const useModelsMock = vi.fn() const useProviderMock = vi.fn() +const moveMock = vi.fn() + +vi.mock('@data/hooks/useReorder', () => ({ + useReorder: () => ({ move: moveMock }) +})) vi.mock('@renderer/hooks/useModel', () => ({ useModels: (...args: any[]) => useModelsMock(...args) @@ -51,6 +56,7 @@ describe('useProviderModelPullReconcile', () => { vi.clearAllMocks() enableProviderWhenModelsAvailableMock.mockResolvedValue(false) useModelsMock.mockReturnValue({ models: [{ id: 'cherryin::model-1' }] }) + moveMock.mockResolvedValue(undefined) useProviderMock.mockReturnValue({ provider: { id: 'cherryin', isEnabled: false }, updateProvider: updateProviderMock @@ -73,6 +79,7 @@ describe('useProviderModelPullReconcile', () => { expect(enableProviderWhenModelsAvailableMock).toHaveBeenCalledWith( { id: 'cherryin', isEnabled: false }, updateProviderMock, + expect.any(Function), 2, 'pull_reconcile_up_to_date' ) @@ -93,6 +100,7 @@ describe('useProviderModelPullReconcile', () => { expect(enableProviderWhenModelsAvailableMock).toHaveBeenCalledWith( { id: 'cherryin', isEnabled: false }, updateProviderMock, + expect.any(Function), 0, 'pull_reconcile_up_to_date' ) diff --git a/src/renderer/pages/settings/ProviderSettings/ModelList/__tests__/usePullReconcileSubmit.test.ts b/src/renderer/pages/settings/ProviderSettings/ModelList/__tests__/usePullReconcileSubmit.test.ts index cff9c386e85..a6da107c02e 100644 --- a/src/renderer/pages/settings/ProviderSettings/ModelList/__tests__/usePullReconcileSubmit.test.ts +++ b/src/renderer/pages/settings/ProviderSettings/ModelList/__tests__/usePullReconcileSubmit.test.ts @@ -9,6 +9,11 @@ const { reconcileTriggerMock } = vi.hoisted(() => ({ })) const useProviderMock = vi.fn() const updateProviderMock = vi.fn() +const moveMock = vi.fn() + +vi.mock('@data/hooks/useReorder', () => ({ + useReorder: () => ({ move: moveMock }) +})) vi.mock('@data/hooks/useDataApi', () => ({ useMutation: () => ({ @@ -43,7 +48,9 @@ describe('usePullReconcileSubmit', () => { reconcileTriggerMock.mockResolvedValue([]) useProviderMock.mockReset() updateProviderMock.mockReset() + moveMock.mockReset() updateProviderMock.mockResolvedValue(undefined) + moveMock.mockResolvedValue(undefined) useProviderMock.mockReturnValue({ provider: { id: 'cherryin', isEnabled: true }, updateProvider: updateProviderMock @@ -123,6 +130,7 @@ describe('usePullReconcileSubmit', () => { }) expect(updateProviderMock).toHaveBeenCalledWith({ isEnabled: true }) + expect(moveMock).toHaveBeenCalledWith('cherryin', { position: 'first' }) }) it('keeps a disabled provider disabled when pull reconcile leaves zero models', async () => { @@ -142,6 +150,7 @@ describe('usePullReconcileSubmit', () => { }) expect(updateProviderMock).not.toHaveBeenCalled() + expect(moveMock).not.toHaveBeenCalled() }) it('surfaces reconcile failure without committing the drawer', async () => { diff --git a/src/renderer/pages/settings/ProviderSettings/ModelList/useProviderModelPullReconcile.ts b/src/renderer/pages/settings/ProviderSettings/ModelList/useProviderModelPullReconcile.ts index 3e6e0282462..ef91203213f 100644 --- a/src/renderer/pages/settings/ProviderSettings/ModelList/useProviderModelPullReconcile.ts +++ b/src/renderer/pages/settings/ProviderSettings/ModelList/useProviderModelPullReconcile.ts @@ -1,3 +1,4 @@ +import { useReorder } from '@data/hooks/useReorder' import { useModels } from '@renderer/hooks/useModel' import { useProvider } from '@renderer/hooks/useProvider' import { useProviderPullReconcile as usePullPreview } from '@renderer/pages/settings/ProviderSettings/hooks/useProviderPullReconcile' @@ -15,7 +16,9 @@ export function useProviderModelPullReconcile(providerId: string) { const pullPreview = usePullPreview(providerId) const [pullReconcileDrawerOpen, setPullReconcileDrawerOpen] = useState(false) const { provider, updateProvider } = useProvider(providerId) + const { move } = useReorder('/providers') const { models } = useModels({ providerId }) + const moveProviderToFirst = useCallback((id: string) => move(id, { position: 'first' }), [move]) const closePullReconcile = useCallback(() => { setPullReconcileDrawerOpen(false) @@ -36,7 +39,13 @@ export function useProviderModelPullReconcile(providerId: string) { if (!hasDiff) { // Up to date: no diff to apply, but the existing local models are valid // for the new key/host — enable the provider if it is currently disabled. - await enableProviderWhenModelsAvailable(provider, updateProvider, models.length, 'pull_reconcile_up_to_date') + await enableProviderWhenModelsAvailable( + provider, + updateProvider, + moveProviderToFirst, + models.length, + 'pull_reconcile_up_to_date' + ) window.toast.success( `${t('settings.models.manage.fetch_up_to_date')} ${t('settings.models.manage.fetch_up_to_date_hint')}` ) @@ -47,7 +56,7 @@ export function useProviderModelPullReconcile(providerId: string) { } catch { /* toast + throw inside fetchPreview */ } - }, [models.length, provider, pullPreview, t, updateProvider]) + }, [models.length, moveProviderToFirst, provider, pullPreview, t, updateProvider]) return { openPullReconcile, diff --git a/src/renderer/pages/settings/ProviderSettings/ModelList/usePullReconcileSubmit.ts b/src/renderer/pages/settings/ProviderSettings/ModelList/usePullReconcileSubmit.ts index 7b74ed63991..a8e5286a97e 100644 --- a/src/renderer/pages/settings/ProviderSettings/ModelList/usePullReconcileSubmit.ts +++ b/src/renderer/pages/settings/ProviderSettings/ModelList/usePullReconcileSubmit.ts @@ -1,4 +1,5 @@ import { useMutation } from '@data/hooks/useDataApi' +import { useReorder } from '@data/hooks/useReorder' import { loggerService } from '@logger' import { useProvider } from '@renderer/hooks/useProvider' import { enableProviderWhenModelsAvailable } from '@renderer/pages/settings/ProviderSettings/utils/providerEnablement' @@ -24,6 +25,8 @@ type UsePullReconcileSubmitOptions = { export function usePullReconcileSubmit({ providerId, onApplyCommitted }: UsePullReconcileSubmitOptions) { const { t } = useTranslation() const { provider, updateProvider } = useProvider(providerId) + const { move } = useReorder('/providers') + const moveProviderToFirst = useCallback((id: string) => move(id, { position: 'first' }), [move]) const { trigger: reconcileTrigger, isLoading: applyBusy } = useMutation( 'POST', '/providers/:providerId/models:reconcile', @@ -44,6 +47,7 @@ export function usePullReconcileSubmit({ providerId, onApplyCommitted }: UsePull await enableProviderWhenModelsAvailable( provider, updateProvider, + moveProviderToFirst, reconciledModels.length, 'pull_reconcile_apply' ) @@ -71,7 +75,7 @@ export function usePullReconcileSubmit({ providerId, onApplyCommitted }: UsePull window.toast.error(t('settings.models.manage.sync_pull_failed')) } }, - [onApplyCommitted, provider, providerId, reconcileTrigger, t, updateProvider] + [onApplyCommitted, moveProviderToFirst, provider, providerId, reconcileTrigger, t, updateProvider] ) return { diff --git a/src/renderer/pages/settings/ProviderSettings/hooks/__tests__/useProviderAutoModelSync.test.tsx b/src/renderer/pages/settings/ProviderSettings/hooks/__tests__/useProviderAutoModelSync.test.tsx index 7d4d17acf33..e9a03e5ba2d 100644 --- a/src/renderer/pages/settings/ProviderSettings/hooks/__tests__/useProviderAutoModelSync.test.tsx +++ b/src/renderer/pages/settings/ProviderSettings/hooks/__tests__/useProviderAutoModelSync.test.tsx @@ -13,6 +13,11 @@ const useModelsMock = vi.fn() const useProviderModelSyncMock = vi.fn() const syncProviderModelsMock = vi.fn() const updateProviderMock = vi.fn() +const moveMock = vi.fn() + +vi.mock('@data/hooks/useReorder', () => ({ + useReorder: () => ({ move: moveMock }) +})) vi.mock('@logger', () => ({ loggerService: { @@ -45,6 +50,7 @@ describe('useProviderAutoModelSync', () => { vi.clearAllMocks() syncProviderModelsMock.mockResolvedValue([]) updateProviderMock.mockResolvedValue(undefined) + moveMock.mockResolvedValue(undefined) useProviderMock.mockReturnValue({ provider: { @@ -113,6 +119,7 @@ describe('useProviderAutoModelSync', () => { renderHook(() => useProviderAutoModelSync('ollama')) await waitFor(() => expect(updateProviderMock).toHaveBeenCalledWith({ isEnabled: true })) + expect(moveMock).toHaveBeenCalledWith('ollama', { position: 'first' }) }) it('syncs only once for the same initial eligible configuration (non-key provider)', async () => { @@ -222,6 +229,7 @@ describe('useProviderAutoModelSync', () => { await waitFor(() => expect(syncProviderModelsMock).toHaveBeenCalledTimes(1)) await waitFor(() => expect(updateProviderMock).toHaveBeenCalledWith({ isEnabled: true })) + expect(moveMock).toHaveBeenCalledWith('claude-code', { position: 'first' }) }) it('does not sync or enable a login provider until it is signed in', async () => { diff --git a/src/renderer/pages/settings/ProviderSettings/hooks/providerSetting/__tests__/useProviderConnectionCheck.test.tsx b/src/renderer/pages/settings/ProviderSettings/hooks/providerSetting/__tests__/useProviderConnectionCheck.test.tsx index 2a798c6070a..1415e2834b0 100644 --- a/src/renderer/pages/settings/ProviderSettings/hooks/providerSetting/__tests__/useProviderConnectionCheck.test.tsx +++ b/src/renderer/pages/settings/ProviderSettings/hooks/providerSetting/__tests__/useProviderConnectionCheck.test.tsx @@ -11,12 +11,17 @@ const useAuthenticationApiKeyMock = vi.fn() const useProviderEndpointsMock = vi.fn() const checkApiMock = vi.fn() const updateProviderMock = vi.fn() +const moveMock = vi.fn() const commitInputApiKeyNowMock = vi.fn() const showErrorDetailPopupMock = vi.fn() const { loggerErrorMock } = vi.hoisted(() => ({ loggerErrorMock: vi.fn() })) +vi.mock('@data/hooks/useReorder', () => ({ + useReorder: () => ({ move: moveMock }) +})) + vi.mock('react-i18next', async (importOriginal) => { const actual = await importOriginal() @@ -76,6 +81,7 @@ describe('useProviderConnectionCheck', () => { success: vi.fn() } + moveMock.mockResolvedValue(undefined) useProviderMock.mockReturnValue({ provider: { id: 'cherryin', name: 'CherryIN', isEnabled: false }, updateProvider: updateProviderMock @@ -158,6 +164,7 @@ describe('useProviderConnectionCheck', () => { }) expect(updateProviderMock).toHaveBeenCalledWith({ isEnabled: true }) + expect(moveMock).toHaveBeenCalledWith('cherryin', { position: 'first' }) }) it('persists the pending API key before running the check and before enabling the provider', async () => { @@ -172,6 +179,7 @@ describe('useProviderConnectionCheck', () => { expect(commitInputApiKeyNowMock).toHaveBeenCalledTimes(1) expect(updateProviderMock).toHaveBeenCalledWith({ isEnabled: true }) + expect(moveMock).toHaveBeenCalledWith('cherryin', { position: 'first' }) // commit must run before the check (so the check validates the pending key, // not a stale saved one) and before enabling the provider. expect(commitInputApiKeyNowMock.mock.invocationCallOrder[0]).toBeLessThan(checkApiMock.mock.invocationCallOrder[0]) @@ -208,6 +216,7 @@ describe('useProviderConnectionCheck', () => { }) it('does not patch an already enabled provider after a successful model connection check', async () => { + moveMock.mockResolvedValue(undefined) useProviderMock.mockReturnValue({ provider: { id: 'cherryin', name: 'CherryIN', isEnabled: true }, updateProvider: updateProviderMock diff --git a/src/renderer/pages/settings/ProviderSettings/hooks/providerSetting/useProviderAutoModelSync.ts b/src/renderer/pages/settings/ProviderSettings/hooks/providerSetting/useProviderAutoModelSync.ts index 707cf04e8d5..aae5cc8fcbc 100644 --- a/src/renderer/pages/settings/ProviderSettings/hooks/providerSetting/useProviderAutoModelSync.ts +++ b/src/renderer/pages/settings/ProviderSettings/hooks/providerSetting/useProviderAutoModelSync.ts @@ -1,3 +1,4 @@ +import { useReorder } from '@data/hooks/useReorder' import { loggerService } from '@logger' import { useModels } from '@renderer/hooks/useModel' import { useProvider, useProviderApiKeys } from '@renderer/hooks/useProvider' @@ -5,7 +6,7 @@ import { enableProviderWhenModelsAvailable } from '@renderer/pages/settings/Prov import { providerNeedsApiKeyForModelSync } from '@renderer/pages/settings/ProviderSettings/utils/providerModelSyncRequirements' import { isExternalCliProvider, isLoginBasedProvider } from '@shared/utils/provider' import { getProviderHostTopology } from '@shared/utils/providerTopology' -import { useEffect, useMemo, useRef } from 'react' +import { useCallback, useEffect, useMemo, useRef } from 'react' import { useProviderModelSync } from '../useProviderModelSync' import { PROVIDER_SETTINGS_MODEL_SWR_OPTIONS } from './constants' @@ -19,6 +20,8 @@ export function useProviderAutoModelSync(providerId: string) { const { data: apiKeysData } = useProviderApiKeys(providerId) const { models } = useModels({ providerId }, { swrOptions: PROVIDER_SETTINGS_MODEL_SWR_OPTIONS }) const { syncProviderModels, isSyncingModels } = useProviderModelSync(providerId, { existingModels: [...models] }) + const { move } = useReorder('/providers') + const moveProviderToFirst = useCallback((id: string) => move(id, { position: 'first' }), [move]) const initialModelSyncSignatureRef = useRef(null) const lastAutoSyncLogKeyRef = useRef(null) @@ -171,7 +174,13 @@ export function useProviderAutoModelSync(providerId: string) { initialModelSyncSignatureRef.current = initialModelSyncSignature void syncProviderModels() .then(async (syncedModels) => { - await enableProviderWhenModelsAvailable(provider, updateProvider, syncedModels.length, 'auto_model_sync') + await enableProviderWhenModelsAvailable( + provider, + updateProvider, + moveProviderToFirst, + syncedModels.length, + 'auto_model_sync' + ) }) .catch((error) => { logger.error('Provider auto model sync failed', { providerId, error }) @@ -179,5 +188,13 @@ export function useProviderAutoModelSync(providerId: string) { initialModelSyncSignatureRef.current = null } }) - }, [autoSyncDecision, initialModelSyncSignature, provider, providerId, syncProviderModels, updateProvider]) + }, [ + autoSyncDecision, + initialModelSyncSignature, + moveProviderToFirst, + provider, + providerId, + syncProviderModels, + updateProvider + ]) } diff --git a/src/renderer/pages/settings/ProviderSettings/hooks/providerSetting/useProviderConnectionCheck.ts b/src/renderer/pages/settings/ProviderSettings/hooks/providerSetting/useProviderConnectionCheck.ts index a142ff610ed..bef21cc75aa 100644 --- a/src/renderer/pages/settings/ProviderSettings/hooks/providerSetting/useProviderConnectionCheck.ts +++ b/src/renderer/pages/settings/ProviderSettings/hooks/providerSetting/useProviderConnectionCheck.ts @@ -1,3 +1,4 @@ +import { useReorder } from '@data/hooks/useReorder' import { loggerService } from '@logger' import { showErrorDetailPopup } from '@renderer/components/ErrorDetailModal' import { useModels } from '@renderer/hooks/useModel' @@ -22,6 +23,7 @@ const logger = loggerService.withContext('ProviderSettings:ConnectionCheck') export function useProviderConnectionCheck(providerId: string) { const { provider, updateProvider } = useProvider(providerId) + const { move } = useReorder('/providers') const [connectionCheckOpen, setConnectionCheckOpen] = useState(false) const { models } = useModels( { providerId }, @@ -38,6 +40,7 @@ export function useProviderConnectionCheck(providerId: string) { }) const checkableModels = models + const moveProviderToFirst = useCallback((id: string) => move(id, { position: 'first' }), [move]) const checkableApiKeys = useMemo(() => splitApiKeyString(formatApiKeys(inputApiKey)).filter(Boolean), [inputApiKey]) // AbortController + runId pair guards against stale callbacks landing on the @@ -117,7 +120,13 @@ export function useProviderConnectionCheck(providerId: string) { // Enable the provider (if disabled) only after a successful check. Enable // swallows its own errors, so it never diverts to the failure path. - await enableProviderWhenModelsAvailable(provider, updateProvider, checkableModels.length, 'connection_check') + await enableProviderWhenModelsAvailable( + provider, + updateProvider, + moveProviderToFirst, + checkableModels.length, + 'connection_check' + ) // The enable await can interleave with a newer check; drop this run if it // was superseded or aborted before touching success state. @@ -162,7 +171,16 @@ export function useProviderConnectionCheck(providerId: string) { setConnectionCheckOpen(false) } }, - [abortInFlightCheck, checkableModels.length, commitInputApiKeyNow, i18n, provider, setTimeoutTimer, updateProvider] + [ + abortInFlightCheck, + checkableModels.length, + commitInputApiKeyNow, + i18n, + moveProviderToFirst, + provider, + setTimeoutTimer, + updateProvider + ] ) const checkApi = useCallback(async () => { diff --git a/src/renderer/pages/settings/ProviderSettings/utils/__tests__/providerEnablement.test.ts b/src/renderer/pages/settings/ProviderSettings/utils/__tests__/providerEnablement.test.ts index 7faf112f3d7..5544de00fd1 100644 --- a/src/renderer/pages/settings/ProviderSettings/utils/__tests__/providerEnablement.test.ts +++ b/src/renderer/pages/settings/ProviderSettings/utils/__tests__/providerEnablement.test.ts @@ -14,53 +14,110 @@ describe('enableProviderWhenModelsAvailable', () => { loggerErrorSpy = vi.spyOn(mockRendererLoggerService, 'error').mockImplementation(() => {}) }) - it('enables a disabled provider when at least one model is available', async () => { + it('enables a disabled provider when at least one model is available and moves it to the top', async () => { const updateProvider = vi.fn().mockResolvedValue(undefined) - - const enabled = await enableProviderWhenModelsAvailable(disabledProvider, updateProvider, 2, 'test') + const moveProviderToFirst = vi.fn().mockResolvedValue(undefined) + + const enabled = await enableProviderWhenModelsAvailable( + disabledProvider, + updateProvider, + moveProviderToFirst, + 2, + 'test' + ) expect(enabled).toBe(true) expect(updateProvider).toHaveBeenCalledWith({ isEnabled: true }) + expect(moveProviderToFirst).toHaveBeenCalledWith('cherryin') }) it('no-ops when the provider is already enabled', async () => { const updateProvider = vi.fn().mockResolvedValue(undefined) - - const enabled = await enableProviderWhenModelsAvailable(enabledProvider, updateProvider, 2, 'test') + const moveProviderToFirst = vi.fn().mockResolvedValue(undefined) + + const enabled = await enableProviderWhenModelsAvailable( + enabledProvider, + updateProvider, + moveProviderToFirst, + 2, + 'test' + ) expect(enabled).toBe(false) expect(updateProvider).not.toHaveBeenCalled() + expect(moveProviderToFirst).not.toHaveBeenCalled() }) it('no-ops when no models are available', async () => { const updateProvider = vi.fn().mockResolvedValue(undefined) - - const enabled = await enableProviderWhenModelsAvailable(disabledProvider, updateProvider, 0, 'test') + const moveProviderToFirst = vi.fn().mockResolvedValue(undefined) + + const enabled = await enableProviderWhenModelsAvailable( + disabledProvider, + updateProvider, + moveProviderToFirst, + 0, + 'test' + ) expect(enabled).toBe(false) expect(updateProvider).not.toHaveBeenCalled() + expect(moveProviderToFirst).not.toHaveBeenCalled() }) it('no-ops when the provider has not resolved yet', async () => { const updateProvider = vi.fn().mockResolvedValue(undefined) + const moveProviderToFirst = vi.fn().mockResolvedValue(undefined) - const enabled = await enableProviderWhenModelsAvailable(undefined, updateProvider, 2, 'test') + const enabled = await enableProviderWhenModelsAvailable(undefined, updateProvider, moveProviderToFirst, 2, 'test') expect(enabled).toBe(false) expect(updateProvider).not.toHaveBeenCalled() + expect(moveProviderToFirst).not.toHaveBeenCalled() }) it('returns false and logs without throwing when the update fails', async () => { const updateError = new Error('patch failed') const updateProvider = vi.fn().mockRejectedValue(updateError) - - const enabled = await enableProviderWhenModelsAvailable(disabledProvider, updateProvider, 2, 'test') + const moveProviderToFirst = vi.fn().mockResolvedValue(undefined) + + const enabled = await enableProviderWhenModelsAvailable( + disabledProvider, + updateProvider, + moveProviderToFirst, + 2, + 'test' + ) expect(enabled).toBe(false) expect(updateProvider).toHaveBeenCalledWith({ isEnabled: true }) + expect(moveProviderToFirst).not.toHaveBeenCalled() expect(loggerErrorSpy).toHaveBeenCalledWith( 'Failed to enable provider when models are available', expect.objectContaining({ providerId: 'cherryin', modelCount: 2, source: 'test', error: updateError }) ) }) + + it('rolls back the enable state when moving the provider to the top fails', async () => { + const moveError = new Error('move failed') + const updateProvider = vi.fn().mockResolvedValue(undefined) + const moveProviderToFirst = vi.fn().mockRejectedValue(moveError) + + const enabled = await enableProviderWhenModelsAvailable( + disabledProvider, + updateProvider, + moveProviderToFirst, + 2, + 'test' + ) + + expect(enabled).toBe(false) + expect(updateProvider).toHaveBeenNthCalledWith(1, { isEnabled: true }) + expect(moveProviderToFirst).toHaveBeenCalledWith('cherryin') + expect(updateProvider).toHaveBeenNthCalledWith(2, { isEnabled: false }) + expect(loggerErrorSpy).toHaveBeenCalledWith( + 'Failed to move enabled provider to the top', + expect.objectContaining({ providerId: 'cherryin', modelCount: 2, source: 'test', error: moveError }) + ) + }) }) diff --git a/src/renderer/pages/settings/ProviderSettings/utils/providerEnablement.ts b/src/renderer/pages/settings/ProviderSettings/utils/providerEnablement.ts index ae7ef40a6f3..18eb0c9ee41 100644 --- a/src/renderer/pages/settings/ProviderSettings/utils/providerEnablement.ts +++ b/src/renderer/pages/settings/ProviderSettings/utils/providerEnablement.ts @@ -4,10 +4,13 @@ import type { Provider } from '@shared/data/types/provider' const logger = loggerService.withContext('ProviderSettings:EnableProviderWhenModelsAvailable') -/** Enables a disabled provider once a flow has confirmed it has usable models. */ +type MoveProviderToFirst = (providerId: Provider['id']) => Promise + +/** Enables a disabled provider once a flow has confirmed it has usable models, then moves it to the top. */ export async function enableProviderWhenModelsAvailable( provider: Pick | undefined, updateProvider: (updates: UpdateProviderDto) => Promise, + moveProviderToFirst: MoveProviderToFirst, modelCount: number, source: string ): Promise { @@ -17,7 +20,6 @@ export async function enableProviderWhenModelsAvailable( try { await updateProvider({ isEnabled: true }) - return true } catch (error) { logger.error('Failed to enable provider when models are available', { providerId: provider.id, @@ -27,4 +29,18 @@ export async function enableProviderWhenModelsAvailable( }) return false } + + try { + await moveProviderToFirst(provider.id) + return true + } catch (error) { + await updateProvider({ isEnabled: false }).catch(() => undefined) + logger.error('Failed to move enabled provider to the top', { + providerId: provider.id, + modelCount, + source, + error + }) + return false + } } From 769073ead8255d8e9860f16298552c04944a3365 Mon Sep 17 00:00:00 2001 From: ousugo Date: Sat, 4 Jul 2026 23:14:27 +0800 Subject: [PATCH 2/3] fix(provider): harden auto-enable pinning --- .../useProviderModelPullReconcile.test.ts | 11 ++- .../__tests__/usePullReconcileSubmit.test.ts | 9 +- .../useProviderModelPullReconcile.ts | 9 +- .../ModelList/usePullReconcileSubmit.ts | 9 +- .../__tests__/useMoveProviderToFirst.test.ts | 47 ++++++++++ .../useProviderAutoModelSync.test.tsx | 11 ++- .../useProviderConnectionCheck.test.tsx | 11 ++- .../useProviderAutoModelSync.ts | 11 ++- .../useProviderConnectionCheck.ts | 9 +- .../hooks/useMoveProviderToFirst.ts | 41 +++++++++ .../__tests__/providerEnablement.test.ts | 90 +++++++++++++------ .../utils/providerEnablement.ts | 22 ++++- 12 files changed, 213 insertions(+), 67 deletions(-) create mode 100644 src/renderer/pages/settings/ProviderSettings/hooks/__tests__/useMoveProviderToFirst.test.ts create mode 100644 src/renderer/pages/settings/ProviderSettings/hooks/useMoveProviderToFirst.ts diff --git a/src/renderer/pages/settings/ProviderSettings/ModelList/__tests__/useProviderModelPullReconcile.test.ts b/src/renderer/pages/settings/ProviderSettings/ModelList/__tests__/useProviderModelPullReconcile.test.ts index 0669a467eb1..826973f9e3d 100644 --- a/src/renderer/pages/settings/ProviderSettings/ModelList/__tests__/useProviderModelPullReconcile.test.ts +++ b/src/renderer/pages/settings/ProviderSettings/ModelList/__tests__/useProviderModelPullReconcile.test.ts @@ -12,8 +12,11 @@ const useModelsMock = vi.fn() const useProviderMock = vi.fn() const moveMock = vi.fn() -vi.mock('@data/hooks/useReorder', () => ({ - useReorder: () => ({ move: moveMock }) +vi.mock('@renderer/pages/settings/ProviderSettings/hooks/useMoveProviderToFirst', () => ({ + useMoveProviderToFirst: () => ({ + assertCanMoveProviderToFirst: vi.fn(), + moveProviderToFirst: moveMock + }) })) vi.mock('@renderer/hooks/useModel', () => ({ @@ -79,7 +82,7 @@ describe('useProviderModelPullReconcile', () => { expect(enableProviderWhenModelsAvailableMock).toHaveBeenCalledWith( { id: 'cherryin', isEnabled: false }, updateProviderMock, - expect.any(Function), + expect.objectContaining({ moveProviderToFirst: expect.any(Function) }), 2, 'pull_reconcile_up_to_date' ) @@ -100,7 +103,7 @@ describe('useProviderModelPullReconcile', () => { expect(enableProviderWhenModelsAvailableMock).toHaveBeenCalledWith( { id: 'cherryin', isEnabled: false }, updateProviderMock, - expect.any(Function), + expect.objectContaining({ moveProviderToFirst: expect.any(Function) }), 0, 'pull_reconcile_up_to_date' ) diff --git a/src/renderer/pages/settings/ProviderSettings/ModelList/__tests__/usePullReconcileSubmit.test.ts b/src/renderer/pages/settings/ProviderSettings/ModelList/__tests__/usePullReconcileSubmit.test.ts index a6da107c02e..76cc341a933 100644 --- a/src/renderer/pages/settings/ProviderSettings/ModelList/__tests__/usePullReconcileSubmit.test.ts +++ b/src/renderer/pages/settings/ProviderSettings/ModelList/__tests__/usePullReconcileSubmit.test.ts @@ -11,8 +11,11 @@ const useProviderMock = vi.fn() const updateProviderMock = vi.fn() const moveMock = vi.fn() -vi.mock('@data/hooks/useReorder', () => ({ - useReorder: () => ({ move: moveMock }) +vi.mock('@renderer/pages/settings/ProviderSettings/hooks/useMoveProviderToFirst', () => ({ + useMoveProviderToFirst: () => ({ + assertCanMoveProviderToFirst: vi.fn(), + moveProviderToFirst: moveMock + }) })) vi.mock('@data/hooks/useDataApi', () => ({ @@ -130,7 +133,7 @@ describe('usePullReconcileSubmit', () => { }) expect(updateProviderMock).toHaveBeenCalledWith({ isEnabled: true }) - expect(moveMock).toHaveBeenCalledWith('cherryin', { position: 'first' }) + expect(moveMock).toHaveBeenCalledWith('cherryin') }) it('keeps a disabled provider disabled when pull reconcile leaves zero models', async () => { diff --git a/src/renderer/pages/settings/ProviderSettings/ModelList/useProviderModelPullReconcile.ts b/src/renderer/pages/settings/ProviderSettings/ModelList/useProviderModelPullReconcile.ts index ef91203213f..fede737ba01 100644 --- a/src/renderer/pages/settings/ProviderSettings/ModelList/useProviderModelPullReconcile.ts +++ b/src/renderer/pages/settings/ProviderSettings/ModelList/useProviderModelPullReconcile.ts @@ -1,6 +1,6 @@ -import { useReorder } from '@data/hooks/useReorder' import { useModels } from '@renderer/hooks/useModel' import { useProvider } from '@renderer/hooks/useProvider' +import { useMoveProviderToFirst } from '@renderer/pages/settings/ProviderSettings/hooks/useMoveProviderToFirst' import { useProviderPullReconcile as usePullPreview } from '@renderer/pages/settings/ProviderSettings/hooks/useProviderPullReconcile' import { enableProviderWhenModelsAvailable } from '@renderer/pages/settings/ProviderSettings/utils/providerEnablement' import { useCallback, useState } from 'react' @@ -16,9 +16,8 @@ export function useProviderModelPullReconcile(providerId: string) { const pullPreview = usePullPreview(providerId) const [pullReconcileDrawerOpen, setPullReconcileDrawerOpen] = useState(false) const { provider, updateProvider } = useProvider(providerId) - const { move } = useReorder('/providers') const { models } = useModels({ providerId }) - const moveProviderToFirst = useCallback((id: string) => move(id, { position: 'first' }), [move]) + const providerReorder = useMoveProviderToFirst() const closePullReconcile = useCallback(() => { setPullReconcileDrawerOpen(false) @@ -42,7 +41,7 @@ export function useProviderModelPullReconcile(providerId: string) { await enableProviderWhenModelsAvailable( provider, updateProvider, - moveProviderToFirst, + providerReorder, models.length, 'pull_reconcile_up_to_date' ) @@ -56,7 +55,7 @@ export function useProviderModelPullReconcile(providerId: string) { } catch { /* toast + throw inside fetchPreview */ } - }, [models.length, moveProviderToFirst, provider, pullPreview, t, updateProvider]) + }, [models.length, providerReorder, provider, pullPreview, t, updateProvider]) return { openPullReconcile, diff --git a/src/renderer/pages/settings/ProviderSettings/ModelList/usePullReconcileSubmit.ts b/src/renderer/pages/settings/ProviderSettings/ModelList/usePullReconcileSubmit.ts index a8e5286a97e..cb088d29b92 100644 --- a/src/renderer/pages/settings/ProviderSettings/ModelList/usePullReconcileSubmit.ts +++ b/src/renderer/pages/settings/ProviderSettings/ModelList/usePullReconcileSubmit.ts @@ -1,7 +1,7 @@ import { useMutation } from '@data/hooks/useDataApi' -import { useReorder } from '@data/hooks/useReorder' import { loggerService } from '@logger' import { useProvider } from '@renderer/hooks/useProvider' +import { useMoveProviderToFirst } from '@renderer/pages/settings/ProviderSettings/hooks/useMoveProviderToFirst' import { enableProviderWhenModelsAvailable } from '@renderer/pages/settings/ProviderSettings/utils/providerEnablement' import { useCallback } from 'react' import { useTranslation } from 'react-i18next' @@ -25,8 +25,7 @@ type UsePullReconcileSubmitOptions = { export function usePullReconcileSubmit({ providerId, onApplyCommitted }: UsePullReconcileSubmitOptions) { const { t } = useTranslation() const { provider, updateProvider } = useProvider(providerId) - const { move } = useReorder('/providers') - const moveProviderToFirst = useCallback((id: string) => move(id, { position: 'first' }), [move]) + const providerReorder = useMoveProviderToFirst() const { trigger: reconcileTrigger, isLoading: applyBusy } = useMutation( 'POST', '/providers/:providerId/models:reconcile', @@ -47,7 +46,7 @@ export function usePullReconcileSubmit({ providerId, onApplyCommitted }: UsePull await enableProviderWhenModelsAvailable( provider, updateProvider, - moveProviderToFirst, + providerReorder, reconciledModels.length, 'pull_reconcile_apply' ) @@ -75,7 +74,7 @@ export function usePullReconcileSubmit({ providerId, onApplyCommitted }: UsePull window.toast.error(t('settings.models.manage.sync_pull_failed')) } }, - [onApplyCommitted, moveProviderToFirst, provider, providerId, reconcileTrigger, t, updateProvider] + [onApplyCommitted, providerReorder, provider, providerId, reconcileTrigger, t, updateProvider] ) return { diff --git a/src/renderer/pages/settings/ProviderSettings/hooks/__tests__/useMoveProviderToFirst.test.ts b/src/renderer/pages/settings/ProviderSettings/hooks/__tests__/useMoveProviderToFirst.test.ts new file mode 100644 index 00000000000..a6df722d3a6 --- /dev/null +++ b/src/renderer/pages/settings/ProviderSettings/hooks/__tests__/useMoveProviderToFirst.test.ts @@ -0,0 +1,47 @@ +import { renderHook } from '@testing-library/react' +import { beforeEach, describe, expect, it, vi } from 'vitest' + +import { ProviderListNotReadyForReorderError, useMoveProviderToFirst } from '../useMoveProviderToFirst' + +const readCacheMock = vi.fn() +const moveMock = vi.fn() + +vi.mock('@data/hooks/useDataApi', () => ({ + useReadCache: () => readCacheMock +})) + +vi.mock('@data/hooks/useReorder', () => ({ + useReorder: () => ({ + move: moveMock, + applyReorderedList: vi.fn(), + isPending: false + }) +})) + +describe('useMoveProviderToFirst', () => { + beforeEach(() => { + vi.clearAllMocks() + moveMock.mockResolvedValue(undefined) + }) + + it('throws before moving when the provider list cache is not ready', async () => { + readCacheMock.mockReturnValue(undefined) + + const { result } = renderHook(() => useMoveProviderToFirst()) + + expect(() => result.current.assertCanMoveProviderToFirst()).toThrow(ProviderListNotReadyForReorderError) + await expect(result.current.moveProviderToFirst('cherryin')).rejects.toThrow(ProviderListNotReadyForReorderError) + expect(moveMock).not.toHaveBeenCalled() + }) + + it('moves a provider to the first position when the provider list cache is ready', async () => { + readCacheMock.mockReturnValue([{ id: 'openai' }, { id: 'cherryin' }]) + + const { result } = renderHook(() => useMoveProviderToFirst()) + + expect(() => result.current.assertCanMoveProviderToFirst()).not.toThrow() + await result.current.moveProviderToFirst('cherryin') + + expect(moveMock).toHaveBeenCalledWith('cherryin', { position: 'first' }) + }) +}) diff --git a/src/renderer/pages/settings/ProviderSettings/hooks/__tests__/useProviderAutoModelSync.test.tsx b/src/renderer/pages/settings/ProviderSettings/hooks/__tests__/useProviderAutoModelSync.test.tsx index e9a03e5ba2d..4b01cc96071 100644 --- a/src/renderer/pages/settings/ProviderSettings/hooks/__tests__/useProviderAutoModelSync.test.tsx +++ b/src/renderer/pages/settings/ProviderSettings/hooks/__tests__/useProviderAutoModelSync.test.tsx @@ -15,8 +15,11 @@ const syncProviderModelsMock = vi.fn() const updateProviderMock = vi.fn() const moveMock = vi.fn() -vi.mock('@data/hooks/useReorder', () => ({ - useReorder: () => ({ move: moveMock }) +vi.mock('@renderer/pages/settings/ProviderSettings/hooks/useMoveProviderToFirst', () => ({ + useMoveProviderToFirst: () => ({ + assertCanMoveProviderToFirst: vi.fn(), + moveProviderToFirst: moveMock + }) })) vi.mock('@logger', () => ({ @@ -119,7 +122,7 @@ describe('useProviderAutoModelSync', () => { renderHook(() => useProviderAutoModelSync('ollama')) await waitFor(() => expect(updateProviderMock).toHaveBeenCalledWith({ isEnabled: true })) - expect(moveMock).toHaveBeenCalledWith('ollama', { position: 'first' }) + expect(moveMock).toHaveBeenCalledWith('ollama') }) it('syncs only once for the same initial eligible configuration (non-key provider)', async () => { @@ -229,7 +232,7 @@ describe('useProviderAutoModelSync', () => { await waitFor(() => expect(syncProviderModelsMock).toHaveBeenCalledTimes(1)) await waitFor(() => expect(updateProviderMock).toHaveBeenCalledWith({ isEnabled: true })) - expect(moveMock).toHaveBeenCalledWith('claude-code', { position: 'first' }) + expect(moveMock).toHaveBeenCalledWith('claude-code') }) it('does not sync or enable a login provider until it is signed in', async () => { diff --git a/src/renderer/pages/settings/ProviderSettings/hooks/providerSetting/__tests__/useProviderConnectionCheck.test.tsx b/src/renderer/pages/settings/ProviderSettings/hooks/providerSetting/__tests__/useProviderConnectionCheck.test.tsx index 1415e2834b0..485571b4cf4 100644 --- a/src/renderer/pages/settings/ProviderSettings/hooks/providerSetting/__tests__/useProviderConnectionCheck.test.tsx +++ b/src/renderer/pages/settings/ProviderSettings/hooks/providerSetting/__tests__/useProviderConnectionCheck.test.tsx @@ -18,8 +18,11 @@ const { loggerErrorMock } = vi.hoisted(() => ({ loggerErrorMock: vi.fn() })) -vi.mock('@data/hooks/useReorder', () => ({ - useReorder: () => ({ move: moveMock }) +vi.mock('@renderer/pages/settings/ProviderSettings/hooks/useMoveProviderToFirst', () => ({ + useMoveProviderToFirst: () => ({ + assertCanMoveProviderToFirst: vi.fn(), + moveProviderToFirst: moveMock + }) })) vi.mock('react-i18next', async (importOriginal) => { @@ -164,7 +167,7 @@ describe('useProviderConnectionCheck', () => { }) expect(updateProviderMock).toHaveBeenCalledWith({ isEnabled: true }) - expect(moveMock).toHaveBeenCalledWith('cherryin', { position: 'first' }) + expect(moveMock).toHaveBeenCalledWith('cherryin') }) it('persists the pending API key before running the check and before enabling the provider', async () => { @@ -179,7 +182,7 @@ describe('useProviderConnectionCheck', () => { expect(commitInputApiKeyNowMock).toHaveBeenCalledTimes(1) expect(updateProviderMock).toHaveBeenCalledWith({ isEnabled: true }) - expect(moveMock).toHaveBeenCalledWith('cherryin', { position: 'first' }) + expect(moveMock).toHaveBeenCalledWith('cherryin') // commit must run before the check (so the check validates the pending key, // not a stale saved one) and before enabling the provider. expect(commitInputApiKeyNowMock.mock.invocationCallOrder[0]).toBeLessThan(checkApiMock.mock.invocationCallOrder[0]) diff --git a/src/renderer/pages/settings/ProviderSettings/hooks/providerSetting/useProviderAutoModelSync.ts b/src/renderer/pages/settings/ProviderSettings/hooks/providerSetting/useProviderAutoModelSync.ts index aae5cc8fcbc..0db076fd0a6 100644 --- a/src/renderer/pages/settings/ProviderSettings/hooks/providerSetting/useProviderAutoModelSync.ts +++ b/src/renderer/pages/settings/ProviderSettings/hooks/providerSetting/useProviderAutoModelSync.ts @@ -1,12 +1,12 @@ -import { useReorder } from '@data/hooks/useReorder' import { loggerService } from '@logger' import { useModels } from '@renderer/hooks/useModel' import { useProvider, useProviderApiKeys } from '@renderer/hooks/useProvider' +import { useMoveProviderToFirst } from '@renderer/pages/settings/ProviderSettings/hooks/useMoveProviderToFirst' import { enableProviderWhenModelsAvailable } from '@renderer/pages/settings/ProviderSettings/utils/providerEnablement' import { providerNeedsApiKeyForModelSync } from '@renderer/pages/settings/ProviderSettings/utils/providerModelSyncRequirements' import { isExternalCliProvider, isLoginBasedProvider } from '@shared/utils/provider' import { getProviderHostTopology } from '@shared/utils/providerTopology' -import { useCallback, useEffect, useMemo, useRef } from 'react' +import { useEffect, useMemo, useRef } from 'react' import { useProviderModelSync } from '../useProviderModelSync' import { PROVIDER_SETTINGS_MODEL_SWR_OPTIONS } from './constants' @@ -20,8 +20,7 @@ export function useProviderAutoModelSync(providerId: string) { const { data: apiKeysData } = useProviderApiKeys(providerId) const { models } = useModels({ providerId }, { swrOptions: PROVIDER_SETTINGS_MODEL_SWR_OPTIONS }) const { syncProviderModels, isSyncingModels } = useProviderModelSync(providerId, { existingModels: [...models] }) - const { move } = useReorder('/providers') - const moveProviderToFirst = useCallback((id: string) => move(id, { position: 'first' }), [move]) + const providerReorder = useMoveProviderToFirst() const initialModelSyncSignatureRef = useRef(null) const lastAutoSyncLogKeyRef = useRef(null) @@ -177,7 +176,7 @@ export function useProviderAutoModelSync(providerId: string) { await enableProviderWhenModelsAvailable( provider, updateProvider, - moveProviderToFirst, + providerReorder, syncedModels.length, 'auto_model_sync' ) @@ -191,7 +190,7 @@ export function useProviderAutoModelSync(providerId: string) { }, [ autoSyncDecision, initialModelSyncSignature, - moveProviderToFirst, + providerReorder, provider, providerId, syncProviderModels, diff --git a/src/renderer/pages/settings/ProviderSettings/hooks/providerSetting/useProviderConnectionCheck.ts b/src/renderer/pages/settings/ProviderSettings/hooks/providerSetting/useProviderConnectionCheck.ts index bef21cc75aa..467f514e685 100644 --- a/src/renderer/pages/settings/ProviderSettings/hooks/providerSetting/useProviderConnectionCheck.ts +++ b/src/renderer/pages/settings/ProviderSettings/hooks/providerSetting/useProviderConnectionCheck.ts @@ -1,9 +1,9 @@ -import { useReorder } from '@data/hooks/useReorder' import { loggerService } from '@logger' import { showErrorDetailPopup } from '@renderer/components/ErrorDetailModal' import { useModels } from '@renderer/hooks/useModel' import { useProvider } from '@renderer/hooks/useProvider' import { useTimer } from '@renderer/hooks/useTimer' +import { useMoveProviderToFirst } from '@renderer/pages/settings/ProviderSettings/hooks/useMoveProviderToFirst' import { type ApiKeyConnectivity, HealthStatus } from '@renderer/pages/settings/ProviderSettings/types/healthCheck' import { checkApi as runCheckApi } from '@renderer/pages/settings/ProviderSettings/utils/healthCheck' import { enableProviderWhenModelsAvailable } from '@renderer/pages/settings/ProviderSettings/utils/providerEnablement' @@ -23,7 +23,7 @@ const logger = loggerService.withContext('ProviderSettings:ConnectionCheck') export function useProviderConnectionCheck(providerId: string) { const { provider, updateProvider } = useProvider(providerId) - const { move } = useReorder('/providers') + const providerReorder = useMoveProviderToFirst() const [connectionCheckOpen, setConnectionCheckOpen] = useState(false) const { models } = useModels( { providerId }, @@ -40,7 +40,6 @@ export function useProviderConnectionCheck(providerId: string) { }) const checkableModels = models - const moveProviderToFirst = useCallback((id: string) => move(id, { position: 'first' }), [move]) const checkableApiKeys = useMemo(() => splitApiKeyString(formatApiKeys(inputApiKey)).filter(Boolean), [inputApiKey]) // AbortController + runId pair guards against stale callbacks landing on the @@ -123,7 +122,7 @@ export function useProviderConnectionCheck(providerId: string) { await enableProviderWhenModelsAvailable( provider, updateProvider, - moveProviderToFirst, + providerReorder, checkableModels.length, 'connection_check' ) @@ -176,7 +175,7 @@ export function useProviderConnectionCheck(providerId: string) { checkableModels.length, commitInputApiKeyNow, i18n, - moveProviderToFirst, + providerReorder, provider, setTimeoutTimer, updateProvider diff --git a/src/renderer/pages/settings/ProviderSettings/hooks/useMoveProviderToFirst.ts b/src/renderer/pages/settings/ProviderSettings/hooks/useMoveProviderToFirst.ts new file mode 100644 index 00000000000..5b21e73aa6c --- /dev/null +++ b/src/renderer/pages/settings/ProviderSettings/hooks/useMoveProviderToFirst.ts @@ -0,0 +1,41 @@ +import { useReadCache } from '@data/hooks/useDataApi' +import { useReorder } from '@data/hooks/useReorder' +import type { Provider } from '@shared/data/types/provider' +import { useCallback, useMemo } from 'react' + +import type { ProviderReorderActions } from '../utils/providerEnablement' + +export class ProviderListNotReadyForReorderError extends Error { + constructor() { + super('Provider list cache is not ready for reorder') + this.name = 'ProviderListNotReadyForReorderError' + } +} + +/** Moves providers through the list cache-aware reorder contract used by ProviderSettings. */ +export function useMoveProviderToFirst(): ProviderReorderActions { + const readCache = useReadCache() + const { move } = useReorder('/providers') + + const assertCanMoveProviderToFirst = useCallback(() => { + if (readCache('/providers') === undefined) { + throw new ProviderListNotReadyForReorderError() + } + }, [readCache]) + + const moveProviderToFirst = useCallback( + async (providerId: Provider['id']) => { + assertCanMoveProviderToFirst() + await move(providerId, { position: 'first' }) + }, + [assertCanMoveProviderToFirst, move] + ) + + return useMemo( + () => ({ + assertCanMoveProviderToFirst, + moveProviderToFirst + }), + [assertCanMoveProviderToFirst, moveProviderToFirst] + ) +} diff --git a/src/renderer/pages/settings/ProviderSettings/utils/__tests__/providerEnablement.test.ts b/src/renderer/pages/settings/ProviderSettings/utils/__tests__/providerEnablement.test.ts index 5544de00fd1..aeaeac06756 100644 --- a/src/renderer/pages/settings/ProviderSettings/utils/__tests__/providerEnablement.test.ts +++ b/src/renderer/pages/settings/ProviderSettings/utils/__tests__/providerEnablement.test.ts @@ -5,6 +5,16 @@ import { enableProviderWhenModelsAvailable } from '../providerEnablement' const disabledProvider = { id: 'cherryin', isEnabled: false } const enabledProvider = { id: 'cherryin', isEnabled: true } +const createProviderReorder = ({ + assertCanMoveProviderToFirst = vi.fn(), + moveProviderToFirst = vi.fn().mockResolvedValue(undefined) +}: { + assertCanMoveProviderToFirst?: ReturnType + moveProviderToFirst?: ReturnType +} = {}) => ({ + assertCanMoveProviderToFirst, + moveProviderToFirst +}) describe('enableProviderWhenModelsAvailable', () => { let loggerErrorSpy: ReturnType @@ -16,105 +26,131 @@ describe('enableProviderWhenModelsAvailable', () => { it('enables a disabled provider when at least one model is available and moves it to the top', async () => { const updateProvider = vi.fn().mockResolvedValue(undefined) - const moveProviderToFirst = vi.fn().mockResolvedValue(undefined) + const providerReorder = createProviderReorder() const enabled = await enableProviderWhenModelsAvailable( disabledProvider, updateProvider, - moveProviderToFirst, + providerReorder, 2, 'test' ) expect(enabled).toBe(true) + expect(providerReorder.assertCanMoveProviderToFirst).toHaveBeenCalledBefore(updateProvider) expect(updateProvider).toHaveBeenCalledWith({ isEnabled: true }) - expect(moveProviderToFirst).toHaveBeenCalledWith('cherryin') + expect(providerReorder.moveProviderToFirst).toHaveBeenCalledWith('cherryin') }) it('no-ops when the provider is already enabled', async () => { const updateProvider = vi.fn().mockResolvedValue(undefined) - const moveProviderToFirst = vi.fn().mockResolvedValue(undefined) + const providerReorder = createProviderReorder() - const enabled = await enableProviderWhenModelsAvailable( - enabledProvider, - updateProvider, - moveProviderToFirst, - 2, - 'test' - ) + const enabled = await enableProviderWhenModelsAvailable(enabledProvider, updateProvider, providerReorder, 2, 'test') expect(enabled).toBe(false) expect(updateProvider).not.toHaveBeenCalled() - expect(moveProviderToFirst).not.toHaveBeenCalled() + expect(providerReorder.assertCanMoveProviderToFirst).not.toHaveBeenCalled() + expect(providerReorder.moveProviderToFirst).not.toHaveBeenCalled() }) it('no-ops when no models are available', async () => { const updateProvider = vi.fn().mockResolvedValue(undefined) - const moveProviderToFirst = vi.fn().mockResolvedValue(undefined) + const providerReorder = createProviderReorder() const enabled = await enableProviderWhenModelsAvailable( disabledProvider, updateProvider, - moveProviderToFirst, + providerReorder, 0, 'test' ) expect(enabled).toBe(false) expect(updateProvider).not.toHaveBeenCalled() - expect(moveProviderToFirst).not.toHaveBeenCalled() + expect(providerReorder.assertCanMoveProviderToFirst).not.toHaveBeenCalled() + expect(providerReorder.moveProviderToFirst).not.toHaveBeenCalled() }) it('no-ops when the provider has not resolved yet', async () => { const updateProvider = vi.fn().mockResolvedValue(undefined) - const moveProviderToFirst = vi.fn().mockResolvedValue(undefined) + const providerReorder = createProviderReorder() + + const enabled = await enableProviderWhenModelsAvailable(undefined, updateProvider, providerReorder, 2, 'test') + + expect(enabled).toBe(false) + expect(updateProvider).not.toHaveBeenCalled() + expect(providerReorder.assertCanMoveProviderToFirst).not.toHaveBeenCalled() + expect(providerReorder.moveProviderToFirst).not.toHaveBeenCalled() + }) + + it('returns false without enabling when provider list reorder is not ready', async () => { + const reorderError = new Error('provider list cache is not ready') + const updateProvider = vi.fn().mockResolvedValue(undefined) + const providerReorder = createProviderReorder({ + assertCanMoveProviderToFirst: vi.fn(() => { + throw reorderError + }) + }) - const enabled = await enableProviderWhenModelsAvailable(undefined, updateProvider, moveProviderToFirst, 2, 'test') + const enabled = await enableProviderWhenModelsAvailable( + disabledProvider, + updateProvider, + providerReorder, + 2, + 'test' + ) expect(enabled).toBe(false) expect(updateProvider).not.toHaveBeenCalled() - expect(moveProviderToFirst).not.toHaveBeenCalled() + expect(providerReorder.moveProviderToFirst).not.toHaveBeenCalled() + expect(loggerErrorSpy).toHaveBeenCalledWith( + 'Provider list is not ready for enabling with pin-to-top', + expect.objectContaining({ providerId: 'cherryin', modelCount: 2, source: 'test', error: reorderError }) + ) }) it('returns false and logs without throwing when the update fails', async () => { const updateError = new Error('patch failed') const updateProvider = vi.fn().mockRejectedValue(updateError) - const moveProviderToFirst = vi.fn().mockResolvedValue(undefined) + const providerReorder = createProviderReorder() const enabled = await enableProviderWhenModelsAvailable( disabledProvider, updateProvider, - moveProviderToFirst, + providerReorder, 2, 'test' ) expect(enabled).toBe(false) expect(updateProvider).toHaveBeenCalledWith({ isEnabled: true }) - expect(moveProviderToFirst).not.toHaveBeenCalled() + expect(providerReorder.moveProviderToFirst).not.toHaveBeenCalled() expect(loggerErrorSpy).toHaveBeenCalledWith( 'Failed to enable provider when models are available', expect.objectContaining({ providerId: 'cherryin', modelCount: 2, source: 'test', error: updateError }) ) }) - it('rolls back the enable state when moving the provider to the top fails', async () => { + it('does not roll back the enable state when moving the provider to the top fails', async () => { const moveError = new Error('move failed') const updateProvider = vi.fn().mockResolvedValue(undefined) - const moveProviderToFirst = vi.fn().mockRejectedValue(moveError) + const providerReorder = createProviderReorder({ + moveProviderToFirst: vi.fn().mockRejectedValue(moveError) + }) const enabled = await enableProviderWhenModelsAvailable( disabledProvider, updateProvider, - moveProviderToFirst, + providerReorder, 2, 'test' ) expect(enabled).toBe(false) - expect(updateProvider).toHaveBeenNthCalledWith(1, { isEnabled: true }) - expect(moveProviderToFirst).toHaveBeenCalledWith('cherryin') - expect(updateProvider).toHaveBeenNthCalledWith(2, { isEnabled: false }) + expect(updateProvider).toHaveBeenCalledOnce() + expect(updateProvider).toHaveBeenCalledWith({ isEnabled: true }) + expect(providerReorder.moveProviderToFirst).toHaveBeenCalledWith('cherryin') expect(loggerErrorSpy).toHaveBeenCalledWith( 'Failed to move enabled provider to the top', expect.objectContaining({ providerId: 'cherryin', modelCount: 2, source: 'test', error: moveError }) diff --git a/src/renderer/pages/settings/ProviderSettings/utils/providerEnablement.ts b/src/renderer/pages/settings/ProviderSettings/utils/providerEnablement.ts index 18eb0c9ee41..a204773103d 100644 --- a/src/renderer/pages/settings/ProviderSettings/utils/providerEnablement.ts +++ b/src/renderer/pages/settings/ProviderSettings/utils/providerEnablement.ts @@ -4,13 +4,16 @@ import type { Provider } from '@shared/data/types/provider' const logger = loggerService.withContext('ProviderSettings:EnableProviderWhenModelsAvailable') -type MoveProviderToFirst = (providerId: Provider['id']) => Promise +export interface ProviderReorderActions { + assertCanMoveProviderToFirst: () => void + moveProviderToFirst: (providerId: Provider['id']) => Promise +} /** Enables a disabled provider once a flow has confirmed it has usable models, then moves it to the top. */ export async function enableProviderWhenModelsAvailable( provider: Pick | undefined, updateProvider: (updates: UpdateProviderDto) => Promise, - moveProviderToFirst: MoveProviderToFirst, + providerReorder: ProviderReorderActions, modelCount: number, source: string ): Promise { @@ -18,6 +21,18 @@ export async function enableProviderWhenModelsAvailable( return false } + try { + providerReorder.assertCanMoveProviderToFirst() + } catch (error) { + logger.error('Provider list is not ready for enabling with pin-to-top', { + providerId: provider.id, + modelCount, + source, + error + }) + return false + } + try { await updateProvider({ isEnabled: true }) } catch (error) { @@ -31,10 +46,9 @@ export async function enableProviderWhenModelsAvailable( } try { - await moveProviderToFirst(provider.id) + await providerReorder.moveProviderToFirst(provider.id) return true } catch (error) { - await updateProvider({ isEnabled: false }).catch(() => undefined) logger.error('Failed to move enabled provider to the top', { providerId: provider.id, modelCount, From d535f22eff12cb432b3e444de0666c9ca5c1fd4c Mon Sep 17 00:00:00 2001 From: ousugo Date: Sun, 5 Jul 2026 11:41:39 +0800 Subject: [PATCH 3/3] fix(provider): use shared pinning for manual enable --- .../__tests__/useProviderEnable.test.tsx | 42 +++++++++++++++---- .../providerSetting/useProviderEnable.ts | 12 ++++-- 2 files changed, 42 insertions(+), 12 deletions(-) diff --git a/src/renderer/pages/settings/ProviderSettings/hooks/providerSetting/__tests__/useProviderEnable.test.tsx b/src/renderer/pages/settings/ProviderSettings/hooks/providerSetting/__tests__/useProviderEnable.test.tsx index c231f60196d..690113412aa 100644 --- a/src/renderer/pages/settings/ProviderSettings/hooks/providerSetting/__tests__/useProviderEnable.test.tsx +++ b/src/renderer/pages/settings/ProviderSettings/hooks/providerSetting/__tests__/useProviderEnable.test.tsx @@ -5,17 +5,20 @@ import { useProviderEnable } from '../useProviderEnable' const useProviderMock = vi.fn() const useProviderMutationsMock = vi.fn() -const useReorderMock = vi.fn() const updateProviderMock = vi.fn().mockResolvedValue(undefined) const moveMock = vi.fn().mockResolvedValue(undefined) +const assertCanMoveProviderToFirstMock = vi.fn() vi.mock('@renderer/hooks/useProvider', () => ({ useProvider: (...args: any[]) => useProviderMock(...args), useProviderMutations: (...args: any[]) => useProviderMutationsMock(...args) })) -vi.mock('@data/hooks/useReorder', () => ({ - useReorder: (...args: any[]) => useReorderMock(...args) +vi.mock('@renderer/pages/settings/ProviderSettings/hooks/useMoveProviderToFirst', () => ({ + useMoveProviderToFirst: () => ({ + assertCanMoveProviderToFirst: assertCanMoveProviderToFirstMock, + moveProviderToFirst: moveMock + }) })) describe('useProviderEnable', () => { @@ -27,9 +30,7 @@ describe('useProviderEnable', () => { useProviderMutationsMock.mockReturnValue({ updateProvider: updateProviderMock }) - useReorderMock.mockReturnValue({ - move: moveMock - }) + assertCanMoveProviderToFirstMock.mockImplementation(() => undefined) }) it('updates only isEnabled when disabling a provider', async () => { @@ -51,7 +52,7 @@ describe('useProviderEnable', () => { }) expect(updateProviderMock).toHaveBeenCalledWith({ isEnabled: true }) - expect(moveMock).toHaveBeenCalledWith('openai', { position: 'first' }) + expect(moveMock).toHaveBeenCalledWith('openai') }) it('does nothing when the provider is missing', async () => { @@ -90,7 +91,32 @@ describe('useProviderEnable', () => { expect(thrown).toBe(moveError) expect(updateProviderMock).toHaveBeenCalledTimes(2) expect(updateProviderMock).toHaveBeenNthCalledWith(1, { isEnabled: true }) - expect(moveMock).toHaveBeenCalledWith('openai', { position: 'first' }) + expect(moveMock).toHaveBeenCalledWith('openai') expect(updateProviderMock).toHaveBeenNthCalledWith(2, { isEnabled: false }) }) + + it('does not enable when pin-to-top preconditions are not ready', async () => { + useProviderMock.mockReturnValue({ + provider: { id: 'openai', isEnabled: false } + }) + const reorderError = new Error('provider list cache is not ready') + assertCanMoveProviderToFirstMock.mockImplementationOnce(() => { + throw reorderError + }) + + const { result } = renderHook(() => useProviderEnable('openai')) + + let thrown: unknown = null + await act(async () => { + try { + await result.current.toggleProviderEnabled(true) + } catch (error) { + thrown = error + } + }) + + expect(thrown).toBe(reorderError) + expect(updateProviderMock).not.toHaveBeenCalled() + expect(moveMock).not.toHaveBeenCalled() + }) }) diff --git a/src/renderer/pages/settings/ProviderSettings/hooks/providerSetting/useProviderEnable.ts b/src/renderer/pages/settings/ProviderSettings/hooks/providerSetting/useProviderEnable.ts index e8f699653c1..ee0d8efd7d7 100644 --- a/src/renderer/pages/settings/ProviderSettings/hooks/providerSetting/useProviderEnable.ts +++ b/src/renderer/pages/settings/ProviderSettings/hooks/providerSetting/useProviderEnable.ts @@ -1,12 +1,12 @@ -import { useReorder } from '@data/hooks/useReorder' import { useProvider, useProviderMutations } from '@renderer/hooks/useProvider' +import { useMoveProviderToFirst } from '@renderer/pages/settings/ProviderSettings/hooks/useMoveProviderToFirst' import { useCallback } from 'react' /** Persists provider enable changes and moves newly enabled providers to the top. */ export function useProviderEnable(providerId: string) { const { provider } = useProvider(providerId) const { updateProvider } = useProviderMutations(providerId) - const { move } = useReorder('/providers') + const providerReorder = useMoveProviderToFirst() const toggleProviderEnabled = useCallback( async (enabled: boolean) => { @@ -15,6 +15,10 @@ export function useProviderEnable(providerId: string) { } const previousEnabled = provider.isEnabled + if (enabled) { + providerReorder.assertCanMoveProviderToFirst() + } + await updateProvider({ isEnabled: enabled }) if (!enabled) { @@ -22,7 +26,7 @@ export function useProviderEnable(providerId: string) { } try { - await move(providerId, { position: 'first' }) + await providerReorder.moveProviderToFirst(providerId) } catch (error) { // Enable + pin-to-top is one user-facing action. If pinning fails after the // enable already committed, roll the enable state back so we don't leave a @@ -32,7 +36,7 @@ export function useProviderEnable(providerId: string) { throw error } }, - [move, provider, providerId, updateProvider] + [provider, providerId, providerReorder, updateProvider] ) return {