diff --git a/.changeset/quiet-knowledge-saves.md b/.changeset/quiet-knowledge-saves.md new file mode 100644 index 00000000000..5f6dd29d7d5 --- /dev/null +++ b/.changeset/quiet-knowledge-saves.md @@ -0,0 +1,5 @@ +--- +"@nocobase/studio": patch +--- + +Preserve changes when knowledge search sections are saved together and discard stale settings reads. diff --git a/packages/apps/studio/client/pages/config/knowledge-search.tsx b/packages/apps/studio/client/pages/config/knowledge-search.tsx index a1533fa4d90..0f9f69c5826 100644 --- a/packages/apps/studio/client/pages/config/knowledge-search.tsx +++ b/packages/apps/studio/client/pages/config/knowledge-search.tsx @@ -105,14 +105,19 @@ function useSaveSettings() { const notify = useNotify(); const queryClient = useQueryClient(); return useMutation({ - mutationFn: (patch: Partial) => { + scope: { id: 'studio-knowledge-search-settings' }, + mutationFn: async (patch: Partial) => { + // A read started before this save must not replace its base configuration. + await queryClient.cancelQueries({ queryKey: knowledgeSearchKey }); const latest = queryClient.getQueryData(knowledgeSearchKey); if (!latest) throw new Error('The knowledge search settings are not loaded.'); return api.updateKnowledgeSearch({ ...latest.settings, ...patch }); }, - onSuccess: (next) => { + onSuccess: async (next) => { + // Also discard reads started while the PUT was in flight. + await queryClient.cancelQueries({ queryKey: knowledgeSearchKey }); queryClient.setQueryData(knowledgeSearchKey, next); notify.success(t('knowledge.searchSettings.saved')); }, diff --git a/packages/apps/studio/e2e/knowledge-search-settings.test.ts b/packages/apps/studio/e2e/knowledge-search-settings.test.ts new file mode 100644 index 00000000000..9fb4ca35d22 --- /dev/null +++ b/packages/apps/studio/e2e/knowledge-search-settings.test.ts @@ -0,0 +1,91 @@ +import type { Route } from '@playwright/test'; + +import type { + KnowledgeSearchConfig, + KnowledgeSearchSettings, +} from '../shared/knowledge.js'; +import { expect, test } from './support/fixtures.ts'; + +// Settings are global: this worker owns a separate Studio and restores every case. +test.use({ isolatedStudio: true, user: 'admin', locale: 'en-US' }); + +for (const order of [ + ['recall', 'chunking'], + ['chunking', 'recall'], +] as const) { + test(`keeps both knowledge settings saves: ${order.join(' then ')}`, async ({ + page, + api, + studio, + }) => { + const original = (await api.get('knowledgeSearch')) + .settings; + const pending: Route[] = []; + const requests: KnowledgeSearchSettings[] = []; + try { + await api.patch('users/me/preferences', { locale: 'en-US' }); + await page.addInitScript( + "localStorage.setItem('nocobase.locale', 'en-US')", + ); + await page.goto(`${studio.baseURL}/config/knowledge-search`, { + waitUntil: 'domcontentloaded', + }); + const limit = page.locator('#kb-search-recall-limit'); + const target = page.locator('#kb-search-chunking-target'); + await expect(limit).toBeVisible(); + const nextLimit = original.recall.limit === 21 ? 22 : 21; + const nextTarget = original.chunking.target === 1300 ? 1400 : 1300; + await limit.fill(String(nextLimit)); + await target.fill(String(nextTarget)); + const save = (kind: string) => + page + .locator(`section[aria-labelledby="kb-search-${kind}"]`) + .getByRole('button', { name: 'Save', exact: true }); + await page.route('**/api/knowledgeSearch', async (route) => { + if (route.request().method() !== 'PUT') return route.continue(); + requests.push( + route.request().postDataJSON() as KnowledgeSearchSettings, + ); + pending.push(route); + }); + await save(order[0]).click(); + await expect.poll(() => pending.length).toBe(1); + await save(order[1]).click(); + await expect(save(order[1])).toBeDisabled(); + expect(pending).toHaveLength(1); + for (let index = 0; index < 2; index++) { + await expect.poll(() => pending.length).toBe(index + 1); + const route = pending[index]!; + const response = page.waitForResponse( + (answer) => answer.request() === route.request(), + ); + await route.continue(); + expect((await response).status()).toBe(200); + await expect( + page + .getByText('Knowledge search settings saved', { exact: true }) + .last(), + ).toBeVisible(); + } + const expected = { + ...original, + recall: { ...original.recall, limit: nextLimit }, + chunking: { ...original.chunking, target: nextTarget }, + }; + expect(requests[1]).toEqual(expected); + expect( + (await api.get('knowledgeSearch')).settings, + ).toEqual(expected); + await page.unroute('**/api/knowledgeSearch'); + await page.reload({ waitUntil: 'domcontentloaded' }); + await expect(limit).toHaveValue(String(nextLimit)); + await expect(target).toHaveValue(String(nextTarget)); + } finally { + await page.unrouteAll({ behavior: 'ignoreErrors' }); + await api.put('knowledgeSearch', original); + expect( + (await api.get('knowledgeSearch')).settings, + ).toEqual(original); + } + }); +} diff --git a/packages/apps/studio/tests/components/knowledge-search-settings.test.tsx b/packages/apps/studio/tests/components/knowledge-search-settings.test.tsx index bb675e219e6..697c43b03bb 100644 --- a/packages/apps/studio/tests/components/knowledge-search-settings.test.tsx +++ b/packages/apps/studio/tests/components/knowledge-search-settings.test.tsx @@ -5,7 +5,7 @@ * saves on its own over the latest settings. */ import { QueryClient, QueryClientProvider } from '@tanstack/react-query'; -import { render, screen, waitFor, within } from '@testing-library/react'; +import { act, render, screen, waitFor, within } from '@testing-library/react'; import userEvent from '@testing-library/user-event'; import type { ReactElement } from 'react'; import { MemoryRouter } from 'react-router'; @@ -17,6 +17,8 @@ import { type KnowledgeSearchSettings, } from '../../shared/knowledge'; +const notifications = vi.hoisted(() => ({ success: vi.fn(), error: vi.fn() })); + const api = { knowledgeSearch: vi.fn<() => Promise>(), updateKnowledgeSearch: @@ -30,7 +32,7 @@ vi.mock('../../client/access/api', async (importOriginal) => ({ useStudioApi: () => api, })); vi.mock('../../client/access/notify', () => ({ - useNotify: () => ({ success: vi.fn(), error: vi.fn() }), + useNotify: () => notifications, })); vi.mock('@nocobase/i18n/client', async (importOriginal) => ({ ...(await importOriginal()), @@ -44,10 +46,11 @@ vi.mock('@nocobase/i18n/client', async (importOriginal) => ({ const { default: KnowledgeSearchSettingsPage } = await import('../../client/pages/config/knowledge-search'); -const wrap = (ui: ReactElement) => ( - +const wrap = ( + ui: ReactElement, + client = new QueryClient({ defaultOptions: { queries: { retry: false } } }), +) => ( + {ui} ); @@ -90,6 +93,8 @@ function config( } beforeEach(() => { + notifications.success.mockClear(); + notifications.error.mockClear(); api.knowledgeSearch.mockReset(); api.updateKnowledgeSearch.mockReset(); api.updateKnowledgeSearch.mockImplementation((settings) => @@ -357,3 +362,133 @@ describe('the knowledge search settings page', () => { ).toBeInTheDocument(); }); }); + +function deferred() { + let resolve!: (value: T) => void; + let reject!: (error: Error) => void; + const promise = new Promise((yes, no) => { + resolve = yes; + reject = no; + }); + return { promise, resolve, reject }; +} + +const searchKey = ['studio', 'knowledge', 'search']; +const saveButton = (name: string) => + section(name).getByRole('button', { name: 'knowledge.searchSettings.save' }); +async function editBoth() { + await screen.findByText('knowledge.searchSettings.recall.title'); + for (const [id, value] of [ + ['kb-search-recall-limit', '21'], + ['kb-search-chunking-target', '1300'], + ]) { + const input = document.getElementById(id!)!; + await userEvent.clear(input); + await userEvent.type(input, value!); + } +} + +describe('coordinating knowledge settings saves', () => { + it.each([ + ['recall', 'chunking'], + ['chunking', 'recall'], + ])('preserves both queued saves: %s then %s', async (first, second) => { + api.knowledgeSearch.mockResolvedValue(config()); + const pending = deferred(); + api.updateKnowledgeSearch.mockImplementationOnce(() => pending.promise); + render(wrap()); + await editBoth(); + await userEvent.click(saveButton(first)); + await userEvent.click(saveButton(second)); + expect(api.updateKnowledgeSearch).toHaveBeenCalledTimes(1); + await act(async () => + pending.resolve( + config({ settings: api.updateKnowledgeSearch.mock.calls[0]![0] }), + ), + ); + await waitFor(() => + expect(api.updateKnowledgeSearch).toHaveBeenCalledTimes(2), + ); + expect(api.updateKnowledgeSearch.mock.calls[1]![0]).toEqual({ + ...DEFAULT_KNOWLEDGE_SEARCH, + recall: { ...DEFAULT_KNOWLEDGE_SEARCH.recall, limit: 21 }, + chunking: { ...DEFAULT_KNOWLEDGE_SEARCH.chunking, target: 1300 }, + }); + await waitFor(() => expect(saveButton(second)).toBeDisabled()); + }); + + it('releases the queue after failure and keeps the failed draft when another section succeeds', async () => { + api.knowledgeSearch.mockResolvedValue(config()); + const pending = deferred(); + api.updateKnowledgeSearch.mockImplementationOnce(() => pending.promise); + render(wrap()); + await editBoth(); + await userEvent.click(saveButton('recall')); + await userEvent.click(saveButton('chunking')); + await act(async () => pending.reject(new Error('Save failed'))); + await waitFor(() => + expect(api.updateKnowledgeSearch).toHaveBeenCalledTimes(2), + ); + await waitFor(() => expect(saveButton('chunking')).toBeDisabled()); + expect(document.getElementById('kb-search-recall-limit')).toHaveValue(21); + expect(saveButton('recall')).toBeEnabled(); + expect(api.updateKnowledgeSearch.mock.calls[1]![0].recall.limit).toBe(20); + expect(notifications.error).toHaveBeenCalledOnce(); + expect(notifications.success).toHaveBeenCalledOnce(); + await userEvent.click(saveButton('recall')); + await waitFor(() => + expect(api.updateKnowledgeSearch).toHaveBeenCalledTimes(3), + ); + expect(api.updateKnowledgeSearch.mock.calls[2]![0].chunking.target).toBe( + 1300, + ); + }); + + it.each(['before', 'during'])( + 'discards a stale read started %s a save', + async (when) => { + const client = new QueryClient({ + defaultOptions: { queries: { retry: false } }, + }); + api.knowledgeSearch.mockResolvedValue(config()); + const stale = deferred(); + const pending = deferred(); + api.updateKnowledgeSearch.mockImplementationOnce(() => pending.promise); + render(wrap(, client)); + await editBoth(); + api.knowledgeSearch.mockImplementationOnce(() => stale.promise); + const refetch = () => client.refetchQueries({ queryKey: searchKey }); + if (when === 'before') void refetch(); + await userEvent.click(saveButton('recall')); + if (when === 'during') void refetch(); + await act(async () => + pending.resolve( + config({ settings: api.updateKnowledgeSearch.mock.calls[0]![0] }), + ), + ); + await waitFor(() => expect(saveButton('recall')).toBeDisabled()); + await act(async () => stale.resolve(config())); + expect( + client.getQueryData(searchKey)?.settings.recall + .limit, + ).toBe(21); + await userEvent.click(saveButton('chunking')); + await waitFor(() => + expect(api.updateKnowledgeSearch).toHaveBeenCalledTimes(2), + ); + expect(api.updateKnowledgeSearch.mock.calls[1]![0].recall.limit).toBe(21); + }, + ); + + it('keeps read-only fields disabled and provides no save actions', async () => { + api.knowledgeSearch.mockResolvedValue(config({ canManage: false })); + render(wrap()); + await screen.findByText('knowledge.searchSettings.recall.title'); + expect(document.getElementById('kb-search-recall-limit')).toBeDisabled(); + expect(document.getElementById('kb-search-chunking-target')).toBeDisabled(); + expect( + screen.queryByRole('button', { name: 'knowledge.searchSettings.save' }), + ).not.toBeInTheDocument(); + expect(api.updateKnowledgeSearch).not.toHaveBeenCalled(); + }); +});