fix(settings): don't blank a working settings view on a background refetch

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 <noreply@anthropic.com>
This commit is contained in:
Sergey Kozyrenko
2026-07-23 01:05:14 +07:00
co-authored by Claude Opus 4.8
parent 66d9c835b7
commit 1308f2d010
7 changed files with 109 additions and 23 deletions
@@ -854,7 +854,7 @@ function SettingsAPITokens() {
</AppHeader>
);
if (isLoading) {
if (isLoading && !data) {
return (
<>
{pageHeader}
@@ -741,7 +741,7 @@ function SettingsPrompt() {
);
}
if (error) {
if (error && !data) {
return (
<>
{pageHeader}
@@ -792,7 +792,7 @@ function SettingsPrompts() {
</AppHeader>
);
if (isLoading) {
if (isLoading && !data) {
return (
<>
{pageHeader}
@@ -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(<SettingsProvider />);
expect(screen.queryByText('Error loading provider data')).not.toBeInTheDocument();
expect(navigate).not.toHaveBeenCalled();
});
});
@@ -1625,7 +1625,7 @@ function SettingsProvider() {
);
}
if (error) {
if (error && !data) {
return (
<>
<AppHeader>
@@ -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<string, boolean> | undefined }));
vi.mock('@apollo/client/react', () => ({
useMutation: () => [vi.fn(), {}],
useQuery: () => ({ data: { settingsProviders: { enabled: state.enabled } } }),
}));
vi.mock('react-router-dom', async (importOriginal) => ({
...(await importOriginal<typeof import('react-router-dom')>()),
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<string, unknown>,
}));
vi.mock('@apollo/client/react', () => ({
useMutation: () => [vi.fn(), {}],
useQuery: () => queryResult.current,
}));
vi.mock('react-router-dom', async (importOriginal) => ({
...(await importOriginal<typeof import('react-router-dom')>()),
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 }) => <div>{children}</div>;
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(<SettingsProvidersHeader />);
@@ -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(<SettingsProvidersHeader />);
@@ -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(
<MemoryRouter>
<SettingsProviders />
</MemoryRouter>,
);
expect(screen.getByText('Seeded Provider')).toBeInTheDocument();
expect(screen.queryByText('Loading providers...')).not.toBeInTheDocument();
});
});
@@ -404,7 +404,7 @@ function SettingsProviders() {
</AppHeader>
);
if (isLoading) {
if (isLoading && !data) {
return (
<>
{pageHeader}