fix(studio): serialize knowledge search settings saves (#10894)

Co-authored-by: 吴昊 <yy_wuhao@163.com>

.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.

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<KnowledgeSearchSettings>) => {
+    scope: { id: 'studio-knowledge-search-settings' },
+    mutationFn: async (patch: Partial<KnowledgeSearchSettings>) => {
+      // A read started before this save must not replace its base configuration.
+      await queryClient.cancelQueries({ queryKey: knowledgeSearchKey });
       const latest =
         queryClient.getQueryData<KnowledgeSearchConfig>(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'));
     },

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<KnowledgeSearchConfig>('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<KnowledgeSearchConfig>('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<KnowledgeSearchConfig>('knowledgeSearch')).settings,
+      ).toEqual(original);
+    }
+  });
+}

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<KnowledgeSearchConfig>>(),
   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<typeof import('@nocobase/i18n/client')>()),
@@ -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) => (
-  <QueryClientProvider
-    client={new QueryClient({ defaultOptions: { queries: { retry: false } } })}
-  >
+const wrap = (
+  ui: ReactElement,
+  client = new QueryClient({ defaultOptions: { queries: { retry: false } } }),
+) => (
+  <QueryClientProvider client={client}>
     <MemoryRouter>{ui}</MemoryRouter>
   </QueryClientProvider>
 );
@@ -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<T>() {
+  let resolve!: (value: T) => void;
+  let reject!: (error: Error) => void;
+  const promise = new Promise<T>((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<KnowledgeSearchConfig>();
+    api.updateKnowledgeSearch.mockImplementationOnce(() => pending.promise);
+    render(wrap(<KnowledgeSearchSettingsPage />));
+    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<KnowledgeSearchConfig>();
+    api.updateKnowledgeSearch.mockImplementationOnce(() => pending.promise);
+    render(wrap(<KnowledgeSearchSettingsPage />));
+    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<KnowledgeSearchConfig>();
+      const pending = deferred<KnowledgeSearchConfig>();
+      api.updateKnowledgeSearch.mockImplementationOnce(() => pending.promise);
+      render(wrap(<KnowledgeSearchSettingsPage />, 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<KnowledgeSearchConfig>(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(<KnowledgeSearchSettingsPage />));
+    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();
+  });
+});