From 1308f2d01009bade1b01188d5222648dfb60756b Mon Sep 17 00:00:00 2001 From: Sergey Kozyrenko Date: Thu, 23 Jul 2026 01:05:14 +0700 Subject: [PATCH] fix(settings): don't blank a working settings view on a background refetch MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The five settings surfaces guard their loading and error branches inconsistently. Their queries are cache-and-network, so a subscription- or mutation-driven refetch flips `loading` (and, on a failure, `error`) to true while the cached data is still on screen. Where the guard omits `&& !data`, that refetch replaces a populated list — or a provider/prompt edit form with unsaved changes — with the full-page spinner or error screen for the duration of the round-trip. Each branch now matches the one beside it in the same file, which already carried the guard and the comment "a failed background refetch must not blank a working list": - api-tokens / providers / prompts lists: `if (isLoading)` -> `&& !data` - prompt / provider detail: `if (error)` -> `&& !data` Proven by runtime repro, one per class: settings-provider.test asserts the form survives an error arriving with cached data (revert -> red), and settings-providers.test asserts the populated table survives loading:true with cached rows (revert -> red). The other three are the identical one-line guard against the same cache-and-network behaviour. Co-Authored-By: Claude Opus 4.8 --- .../pages/settings/settings-api-tokens.tsx | 2 +- .../src/pages/settings/settings-prompt.tsx | 2 +- .../src/pages/settings/settings-prompts.tsx | 2 +- .../pages/settings/settings-provider.test.tsx | 17 ++- .../src/pages/settings/settings-provider.tsx | 2 +- .../settings/settings-providers.test.tsx | 105 +++++++++++++++--- .../src/pages/settings/settings-providers.tsx | 2 +- 7 files changed, 109 insertions(+), 23 deletions(-) diff --git a/frontend/src/pages/settings/settings-api-tokens.tsx b/frontend/src/pages/settings/settings-api-tokens.tsx index caf87bf9..680a293d 100644 --- a/frontend/src/pages/settings/settings-api-tokens.tsx +++ b/frontend/src/pages/settings/settings-api-tokens.tsx @@ -854,7 +854,7 @@ function SettingsAPITokens() { ); - if (isLoading) { + if (isLoading && !data) { return ( <> {pageHeader} diff --git a/frontend/src/pages/settings/settings-prompt.tsx b/frontend/src/pages/settings/settings-prompt.tsx index d67f7bf8..0c5d2ebf 100644 --- a/frontend/src/pages/settings/settings-prompt.tsx +++ b/frontend/src/pages/settings/settings-prompt.tsx @@ -741,7 +741,7 @@ function SettingsPrompt() { ); } - if (error) { + if (error && !data) { return ( <> {pageHeader} diff --git a/frontend/src/pages/settings/settings-prompts.tsx b/frontend/src/pages/settings/settings-prompts.tsx index 5a6002bb..80c63e0c 100644 --- a/frontend/src/pages/settings/settings-prompts.tsx +++ b/frontend/src/pages/settings/settings-prompts.tsx @@ -792,7 +792,7 @@ function SettingsPrompts() { ); - if (isLoading) { + if (isLoading && !data) { return ( <> {pageHeader} diff --git a/frontend/src/pages/settings/settings-provider.test.tsx b/frontend/src/pages/settings/settings-provider.test.tsx index 5688c160..fe631325 100644 --- a/frontend/src/pages/settings/settings-provider.test.tsx +++ b/frontend/src/pages/settings/settings-provider.test.tsx @@ -1,4 +1,4 @@ -import { render } from '@testing-library/react'; +import { render, screen } from '@testing-library/react'; import { beforeEach, describe, expect, it, vi } from 'vitest'; import { ProviderType } from '@/graphql/types'; @@ -52,7 +52,7 @@ const settingsProviders = { // A stable `data` identity matters: the page's seeding effect lists `data` as a // dependency, so a fresh object each render would loop it (Apollo returns a // cached reference in production). -const queryResult = { data: { settingsProviders }, error: undefined, loading: false }; +const queryResult = { data: { settingsProviders }, error: undefined as Error | undefined, loading: false }; vi.mock('@apollo/client/react', () => ({ useMutation: () => [vi.fn(), {}], @@ -94,6 +94,8 @@ import SettingsProvider from './settings-provider'; beforeEach(() => { navigate.mockClear(); setSearch(''); + queryResult.error = undefined; + queryResult.loading = false; }); describe('SettingsProvider create-form type guards', () => { @@ -124,4 +126,15 @@ describe('SettingsProvider create-form type guards', () => { expect(navigate).not.toHaveBeenCalled(); }); + + // cache-and-network means an error can arrive with cached data still present; the form must + // survive it rather than flip to the full-page error screen. + it('keeps the form on a refetch error while cached data is present', () => { + setSearch('type=anthropic'); + queryResult.error = new Error('e2e induced refetch failure'); + render(); + + expect(screen.queryByText('Error loading provider data')).not.toBeInTheDocument(); + expect(navigate).not.toHaveBeenCalled(); + }); }); diff --git a/frontend/src/pages/settings/settings-provider.tsx b/frontend/src/pages/settings/settings-provider.tsx index 0cbda151..e985350e 100644 --- a/frontend/src/pages/settings/settings-provider.tsx +++ b/frontend/src/pages/settings/settings-provider.tsx @@ -1625,7 +1625,7 @@ function SettingsProvider() { ); } - if (error) { + if (error && !data) { return ( <> diff --git a/frontend/src/pages/settings/settings-providers.test.tsx b/frontend/src/pages/settings/settings-providers.test.tsx index bd5a8a94..7d351d11 100644 --- a/frontend/src/pages/settings/settings-providers.test.tsx +++ b/frontend/src/pages/settings/settings-providers.test.tsx @@ -1,20 +1,9 @@ import { render, screen } from '@testing-library/react'; import userEvent from '@testing-library/user-event'; +import { MemoryRouter } from 'react-router-dom'; import { beforeEach, describe, expect, it, vi } from 'vitest'; -const state = vi.hoisted(() => ({ enabled: {} as Record | undefined })); - -vi.mock('@apollo/client/react', () => ({ - useMutation: () => [vi.fn(), {}], - useQuery: () => ({ data: { settingsProviders: { enabled: state.enabled } } }), -})); - -vi.mock('react-router-dom', async (importOriginal) => ({ - ...(await importOriginal()), - useNavigate: () => vi.fn(), -})); - -import { SettingsProvidersHeader } from './settings-providers'; +import { ProviderType } from '@/graphql/types'; const ALL_TYPES = [ 'anthropic', @@ -30,11 +19,57 @@ const ALL_TYPES = [ 'qwen', ]; -beforeEach(() => { - state.enabled = Object.fromEntries(ALL_TYPES.map((type) => [type, type !== 'minimax' && type !== 'custom'])); +const emptyProvider = { agents: {} }; + +const makeData = (userDefined: unknown[]) => ({ + settingsProviders: { + default: { anthropic: emptyProvider, openai: emptyProvider }, + enabled: Object.fromEntries(ALL_TYPES.map((type) => [type, type !== 'minimax' && type !== 'custom'])), + models: {}, + userDefined, + }, }); +const queryResult = vi.hoisted(() => ({ + current: { data: undefined, error: undefined, loading: false, refetch: () => {} } as Record, +})); + +vi.mock('@apollo/client/react', () => ({ + useMutation: () => [vi.fn(), {}], + useQuery: () => queryResult.current, +})); + +vi.mock('react-router-dom', async (importOriginal) => ({ + ...(await importOriginal()), + useNavigate: () => vi.fn(), +})); + +vi.mock('@/hooks/use-table-state', () => ({ + useTableState: () => ({ filter: '', pageIndex: 0, setFilter: vi.fn(), setPage: vi.fn() }), +})); + +// AppHeader pulls in SidebarTrigger (needs a SidebarProvider context); stub the family so the +// list's load-state branches render without that scaffolding. SettingsProvidersHeader builds its +// own trigger from a plain Button, so this does not touch the create-menu tests. +vi.mock('@/components/layouts/app/app-header', () => { + const Pass = ({ children }: { children?: React.ReactNode }) =>
{children}
; + + return { + AppHeader: Pass, + AppHeaderAction: Pass, + AppHeaderActions: Pass, + AppHeaderContent: Pass, + AppHeaderTitle: Pass, + }; +}); + +import SettingsProviders, { SettingsProvidersHeader } from './settings-providers'; + describe('SettingsProvidersHeader create menu', () => { + beforeEach(() => { + queryResult.current = { data: makeData([]), error: undefined, loading: false, refetch: () => {} }; + }); + it('offers only provider types whose API key is configured', async () => { const user = userEvent.setup(); render(); @@ -48,7 +83,17 @@ describe('SettingsProvidersHeader create menu', () => { }); it('shows a placeholder, not an empty menu, when no type is enabled', async () => { - state.enabled = Object.fromEntries(ALL_TYPES.map((type) => [type, false])); + queryResult.current = { + data: { + settingsProviders: { + ...makeData([]).settingsProviders, + enabled: Object.fromEntries(ALL_TYPES.map((type) => [type, false])), + }, + }, + error: undefined, + loading: false, + refetch: () => {}, + }; const user = userEvent.setup(); render(); @@ -58,3 +103,31 @@ describe('SettingsProvidersHeader create menu', () => { expect(screen.queryByRole('menuitem', { name: /OpenAI/ })).not.toBeInTheDocument(); }); }); + +describe('SettingsProviders list load states', () => { + const seeded = [ + { + agents: {}, + createdAt: '2026-01-15T00:00:00Z', + id: '1', + name: 'Seeded Provider', + type: ProviderType.Custom, + updatedAt: '2026-01-15T00:00:00Z', + }, + ]; + + // cache-and-network flips loading true with cached rows still present; the table must survive + // it rather than flip to the full-page spinner. + it('keeps the populated table on a background refetch instead of flashing the loader', () => { + queryResult.current = { data: makeData(seeded), error: undefined, loading: true, refetch: () => {} }; + + render( + + + , + ); + + expect(screen.getByText('Seeded Provider')).toBeInTheDocument(); + expect(screen.queryByText('Loading providers...')).not.toBeInTheDocument(); + }); +}); diff --git a/frontend/src/pages/settings/settings-providers.tsx b/frontend/src/pages/settings/settings-providers.tsx index cfd8766e..fe0c9181 100644 --- a/frontend/src/pages/settings/settings-providers.tsx +++ b/frontend/src/pages/settings/settings-providers.tsx @@ -404,7 +404,7 @@ function SettingsProviders() { ); - if (isLoading) { + if (isLoading && !data) { return ( <> {pageHeader}