From 309753b799517cd5ed11e9ae25fcec924be34751 Mon Sep 17 00:00:00 2001 From: Maurice Date: Fri, 9 Oct 2026 09:59:08 +0200 Subject: [PATCH] refactor(settings): share the AI parsing form between desktop and phone The provider, model, key and multimodal state, the hydration from the settings store and the save with its reload on failure now live in one useLlmConnection hook. The phone keeps its picker sheet state. --- .../Settings/LlmConnectionSection.tsx | 90 ++++------------- .../Settings/useLlmConnection.test.ts | 96 ++++++++++++++++++ .../components/Settings/useLlmConnection.ts | 98 +++++++++++++++++++ .../settings/MLlmConnectionSection.tsx | 88 ++++------------- 4 files changed, 230 insertions(+), 142 deletions(-) create mode 100644 client/src/components/Settings/useLlmConnection.test.ts create mode 100644 client/src/components/Settings/useLlmConnection.ts diff --git a/client/src/components/Settings/LlmConnectionSection.tsx b/client/src/components/Settings/LlmConnectionSection.tsx index f6db68fa4..34701c629 100644 --- a/client/src/components/Settings/LlmConnectionSection.tsx +++ b/client/src/components/Settings/LlmConnectionSection.tsx @@ -1,17 +1,13 @@ -import React, { useEffect, useMemo, useState } from 'react' +import React from 'react' import { Sparkles, Save } from 'lucide-react' import { useTranslation } from '../../i18n' -import { useToast } from '../shared/Toast' -import { useSettingsStore } from '../../store/settingsStore' -import type { Settings } from '../../types' +import { useLlmConnection, type LlmProvider as Provider } from './useLlmConnection' import Section from './Section' import ToggleSwitch from './ToggleSwitch' import { SETTINGS_BUTTON_PRIMARY, SettingRow, SettingRows, SettingsHint } from './settingsKit' import { EditorField, GRID_2, INPUT, Segmented } from '../shared/dialogParts' import { fs } from '../shared/DialogShell' -type Provider = NonNullable - /** * Settings → Integrations → AI parsing. Per-user model used to extract bookings * from uploaded files. It only takes effect when the admin has not configured an @@ -29,71 +25,21 @@ type Provider = NonNullable */ export default function LlmConnectionSection(): React.ReactElement { const { t } = useTranslation() - const toast = useToast() - const settings = useSettingsStore(s => s.settings) - const isLoaded = useSettingsStore(s => s.isLoaded) - const updateSettings = useSettingsStore(s => s.updateSettings) - const loadSettings = useSettingsStore(s => s.loadSettings) - - const [provider, setProvider] = useState('openai') - const [model, setModel] = useState('') - const [apiKey, setApiKey] = useState('') - const [multimodal, setMultimodal] = useState(false) - const [hasStoredKey, setHasStoredKey] = useState(false) - const [saving, setSaving] = useState(false) - - // Hydrate from the loaded settings. llm_api_key arrives masked, so we only use - // its presence to drive the placeholder, never the value itself. A stored - // 'local' from before #1772 shows as OpenAI (local state only, nothing is - // saved until Save is pressed) so the form never offers a value the server - // would refuse. - useEffect(() => { - if (!isLoaded) return - const stored = settings.llm_provider || 'openai' - setProvider(stored === 'local' ? 'openai' : stored) - setModel(settings.llm_model || '') - setMultimodal(settings.llm_multimodal === true) - setHasStoredKey(!!settings.llm_api_key) - }, [isLoaded, settings.llm_provider, settings.llm_model, settings.llm_multimodal, settings.llm_api_key]) - - const providerOptions = useMemo( - () => [ - { value: 'openai' as const, label: t('settings.aiParsing.providerOpenai') }, - { value: 'anthropic' as const, label: t('settings.aiParsing.providerAnthropic') }, - ], - [t], - ) - - const handleSave = async () => { - setSaving(true) - try { - const payload: Partial = { - llm_provider: provider, - llm_model: model.trim(), - // Always cleared: the endpoint is instance configuration now, and this - // also drops a value left over from before #1772. - llm_base_url: '', - llm_multimodal: multimodal, - } - // Send the key only when the user typed a new one — a blank field means - // "keep the stored key". - const key = apiKey.trim() - if (key) payload.llm_api_key = key - await updateSettings(payload) - setApiKey('') - if (key) setHasStoredKey(true) - toast.success(t('settings.aiParsing.toast.saved')) - } catch { - // updateSettings patches the store before the request and keeps the patch - // when the request fails, so a refused save (the 403 from #1772, or any - // other error) would leave the form showing a value the server never - // stored. Pull the stored settings back in so what is on screen is real. - await loadSettings() - toast.error(t('settings.aiParsing.toast.saveError')) - } finally { - setSaving(false) - } - } + const { + isLoaded, + provider, + setProvider, + providerOptions, + model, + setModel, + apiKey, + setApiKey, + multimodal, + toggleMultimodal, + hasStoredKey, + saving, + handleSave, + } = useLlmConnection() return (
@@ -141,7 +87,7 @@ export default function LlmConnectionSection(): React.ReactElement { setMultimodal(v => !v)} label={t('settings.aiParsing.multimodal')} />} + control={} /> diff --git a/client/src/components/Settings/useLlmConnection.test.ts b/client/src/components/Settings/useLlmConnection.test.ts new file mode 100644 index 000000000..5a4c8fdd4 --- /dev/null +++ b/client/src/components/Settings/useLlmConnection.test.ts @@ -0,0 +1,96 @@ +// FE-COMP-LLM-HOOK-001 to -006: the AI parsing form both settings shells share. +import { act, renderHook } from '@testing-library/react'; +import { buildSettings } from '../../../tests/helpers/factories'; +import { resetAllStores, seedStore } from '../../../tests/helpers/store'; +import { useSettingsStore } from '../../store/settingsStore'; +import type { Settings } from '../../types'; +import { useLlmConnection } from './useLlmConnection'; + +const toast = vi.hoisted(() => ({ success: vi.fn(), error: vi.fn(), warning: vi.fn(), info: vi.fn() })); +vi.mock('../shared/Toast', () => ({ useToast: () => toast })); +vi.mock('../../i18n', () => ({ useTranslation: () => ({ t: (k: string) => k }) })); + +function seed(over: Partial = {}, isLoaded = true) { + const updateSettings = vi.fn().mockResolvedValue(undefined); + const loadSettings = vi.fn().mockResolvedValue(undefined); + seedStore(useSettingsStore, { settings: buildSettings(over), isLoaded, updateSettings, loadSettings }); + return { updateSettings, loadSettings }; +} + +beforeEach(() => { + resetAllStores(); + vi.clearAllMocks(); +}); + +describe('useLlmConnection', () => { + it('FE-COMP-LLM-HOOK-001: waits for the settings, then hydrates them without the key itself', () => { + seed({ llm_provider: 'anthropic', llm_model: 'claude', llm_multimodal: true, llm_api_key: '****' }, false); + const { result, rerender } = renderHook(() => useLlmConnection()); + expect(result.current.provider).toBe('openai'); + expect(result.current.model).toBe(''); + act(() => useSettingsStore.setState({ isLoaded: true })); + rerender(); + expect(result.current.provider).toBe('anthropic'); + expect(result.current.model).toBe('claude'); + expect(result.current.multimodal).toBe(true); + expect(result.current.hasStoredKey).toBe(true); + expect(result.current.apiKey).toBe(''); + }); + + it('FE-COMP-LLM-HOOK-002: a stored local provider from before #1772 shows as OpenAI', () => { + seed({ llm_provider: 'local' }); + const { result } = renderHook(() => useLlmConnection()); + expect(result.current.provider).toBe('openai'); + }); + + it('FE-COMP-LLM-HOOK-003: offers exactly the two hosted providers', () => { + seed(); + const { result } = renderHook(() => useLlmConnection()); + expect(result.current.providerOptions).toEqual([ + { value: 'openai', label: 'settings.aiParsing.providerOpenai' }, + { value: 'anthropic', label: 'settings.aiParsing.providerAnthropic' }, + ]); + }); + + it('FE-COMP-LLM-HOOK-004: save with a blank key keeps the stored one and always clears the endpoint', async () => { + const { updateSettings } = seed({ llm_provider: 'openai', llm_model: 'm' }); + const { result } = renderHook(() => useLlmConnection()); + act(() => result.current.setModel(' gpt ')); + act(() => result.current.toggleMultimodal()); + await act(() => result.current.handleSave()); + expect(updateSettings).toHaveBeenCalledWith({ + llm_provider: 'openai', + llm_model: 'gpt', + llm_base_url: '', + llm_multimodal: true, + }); + expect(toast.success).toHaveBeenCalledWith('settings.aiParsing.toast.saved'); + expect(result.current.saving).toBe(false); + }); + + it('FE-COMP-LLM-HOOK-005: a typed key is sent trimmed, cleared afterwards and marks a stored key', async () => { + const { updateSettings } = seed(); + const { result } = renderHook(() => useLlmConnection()); + act(() => result.current.setProvider('anthropic')); + act(() => result.current.setApiKey(' sk ')); + await act(() => result.current.handleSave()); + expect(updateSettings).toHaveBeenCalledWith( + expect.objectContaining({ llm_provider: 'anthropic', llm_api_key: 'sk' }) + ); + expect(result.current.apiKey).toBe(''); + expect(result.current.hasStoredKey).toBe(true); + }); + + it('FE-COMP-LLM-HOOK-006: a refused save reloads the stored settings and toasts the error', async () => { + const { updateSettings, loadSettings } = seed(); + updateSettings.mockRejectedValue(new Error('403')); + const { result } = renderHook(() => useLlmConnection()); + act(() => result.current.setApiKey('sk')); + await act(() => result.current.handleSave()); + expect(loadSettings).toHaveBeenCalled(); + expect(toast.error).toHaveBeenCalledWith('settings.aiParsing.toast.saveError'); + expect(toast.success).not.toHaveBeenCalled(); + expect(result.current.apiKey).toBe('sk'); + expect(result.current.saving).toBe(false); + }); +}); diff --git a/client/src/components/Settings/useLlmConnection.ts b/client/src/components/Settings/useLlmConnection.ts new file mode 100644 index 000000000..79983e142 --- /dev/null +++ b/client/src/components/Settings/useLlmConnection.ts @@ -0,0 +1,98 @@ +import { useEffect, useMemo, useState } from 'react'; +import { useTranslation } from '../../i18n'; +import { useSettingsStore } from '../../store/settingsStore'; +import type { Settings } from '../../types'; +import { useToast } from '../shared/Toast'; + +export type LlmProvider = NonNullable; + +/** + * The per-user AI parsing model form behind both settings shells (the desktop + * section and its phone twin render their own markup over this). The key is never + * prefilled: a blank field keeps the stored key. Only the two hosted providers are + * offered (#1772), the endpoint is instance configuration. + */ +export function useLlmConnection() { + const { t } = useTranslation(); + const toast = useToast(); + const settings = useSettingsStore((s) => s.settings); + const isLoaded = useSettingsStore((s) => s.isLoaded); + const updateSettings = useSettingsStore((s) => s.updateSettings); + const loadSettings = useSettingsStore((s) => s.loadSettings); + + const [provider, setProvider] = useState('openai'); + const [model, setModel] = useState(''); + const [apiKey, setApiKey] = useState(''); + const [multimodal, setMultimodal] = useState(false); + const [hasStoredKey, setHasStoredKey] = useState(false); + const [saving, setSaving] = useState(false); + + // Hydrate from the loaded settings. llm_api_key arrives masked, so we only use + // its presence to drive the placeholder, never the value itself. A stored + // 'local' from before #1772 shows as OpenAI (local state only, nothing is + // saved until Save is pressed) so the form never offers a value the server + // would refuse. + useEffect(() => { + if (!isLoaded) return; + const stored = settings.llm_provider || 'openai'; + setProvider(stored === 'local' ? 'openai' : stored); + setModel(settings.llm_model || ''); + setMultimodal(settings.llm_multimodal === true); + setHasStoredKey(!!settings.llm_api_key); + }, [isLoaded, settings.llm_provider, settings.llm_model, settings.llm_multimodal, settings.llm_api_key]); + + const providerOptions = useMemo( + () => [ + { value: 'openai' as const, label: t('settings.aiParsing.providerOpenai') }, + { value: 'anthropic' as const, label: t('settings.aiParsing.providerAnthropic') }, + ], + [t] + ); + + const handleSave = async () => { + setSaving(true); + try { + const payload: Partial = { + llm_provider: provider, + llm_model: model.trim(), + // Always cleared: the endpoint is instance configuration now, and this + // also drops a value left over from before #1772. + llm_base_url: '', + llm_multimodal: multimodal, + }; + // Send the key only when the user typed a new one, a blank field means + // "keep the stored key". + const key = apiKey.trim(); + if (key) payload.llm_api_key = key; + await updateSettings(payload); + setApiKey(''); + if (key) setHasStoredKey(true); + toast.success(t('settings.aiParsing.toast.saved')); + } catch { + // updateSettings patches the store before the request and keeps the patch + // when the request fails, so a refused save (the 403 from #1772, or any + // other error) would leave the form showing a value the server never + // stored. Pull the stored settings back in so what is on screen is real. + await loadSettings(); + toast.error(t('settings.aiParsing.toast.saveError')); + } finally { + setSaving(false); + } + }; + + return { + isLoaded, + provider, + setProvider, + providerOptions, + model, + setModel, + apiKey, + setApiKey, + multimodal, + toggleMultimodal: () => setMultimodal((v) => !v), + hasStoredKey, + saving, + handleSave, + }; +} diff --git a/client/src/mobile/screens/settings/MLlmConnectionSection.tsx b/client/src/mobile/screens/settings/MLlmConnectionSection.tsx index a473e4c2c..e8bd52d45 100644 --- a/client/src/mobile/screens/settings/MLlmConnectionSection.tsx +++ b/client/src/mobile/screens/settings/MLlmConnectionSection.tsx @@ -1,15 +1,11 @@ -import React, { useEffect, useMemo, useState } from 'react' +import React, { useState } from 'react' import { Sparkles, Save, ChevronDown } from 'lucide-react' import { useTranslation } from '../../../i18n' -import { useToast } from '../../../components/shared/Toast' -import { useSettingsStore } from '../../../store/settingsStore' -import type { Settings } from '../../../types' +import { useLlmConnection, type LlmProvider as Provider } from '../../../components/Settings/useLlmConnection' import { MSetCard, MSetEyebrow, MSetSelectRow, MSetRow, MSetInput, MSetButton, MSetHint } from './MSettingsUi' import MToggle from '../../components/MToggle' import MSetPickerSheet from './MSetPickerSheet' -type Provider = NonNullable - /** * Mobile-native twin of components/Settings/LlmConnectionSection. Same per-user * AI-parsing model logic (provider/model/key/multimodal, key never prefilled, @@ -19,73 +15,25 @@ type Provider = NonNullable */ export default function MLlmConnectionSection(): React.ReactElement { const { t } = useTranslation() - const toast = useToast() - const settings = useSettingsStore(s => s.settings) - const isLoaded = useSettingsStore(s => s.isLoaded) - const updateSettings = useSettingsStore(s => s.updateSettings) - const loadSettings = useSettingsStore(s => s.loadSettings) - - const [provider, setProvider] = useState('openai') - const [model, setModel] = useState('') - const [apiKey, setApiKey] = useState('') - const [multimodal, setMultimodal] = useState(false) - const [hasStoredKey, setHasStoredKey] = useState(false) - const [saving, setSaving] = useState(false) + const { + isLoaded, + provider, + setProvider, + providerOptions, + model, + setModel, + apiKey, + setApiKey, + multimodal, + toggleMultimodal, + hasStoredKey, + saving, + handleSave, + } = useLlmConnection() const [providerOpen, setProviderOpen] = useState(false) - // Hydrate from the loaded settings. llm_api_key arrives masked, so we only use - // its presence to drive the placeholder, never the value itself. A stored - // 'local' from before #1772 shows as OpenAI (local state only, nothing is - // saved until Save is pressed). - useEffect(() => { - if (!isLoaded) return - const stored = settings.llm_provider || 'openai' - setProvider(stored === 'local' ? 'openai' : stored) - setModel(settings.llm_model || '') - setMultimodal(settings.llm_multimodal === true) - setHasStoredKey(!!settings.llm_api_key) - }, [isLoaded, settings.llm_provider, settings.llm_model, settings.llm_multimodal, settings.llm_api_key]) - - const providerOptions = useMemo( - () => [ - { value: 'openai', label: t('settings.aiParsing.providerOpenai') }, - { value: 'anthropic', label: t('settings.aiParsing.providerAnthropic') }, - ], - [t], - ) const providerLabel = providerOptions.find(o => o.value === provider)?.label ?? provider - const handleSave = async () => { - setSaving(true) - try { - const payload: Partial = { - llm_provider: provider, - llm_model: model.trim(), - // Always cleared: the endpoint is instance configuration now, and this - // also drops a value left over from before #1772. - llm_base_url: '', - llm_multimodal: multimodal, - } - // Send the key only when the user typed a new one — a blank field means - // "keep the stored key". - const key = apiKey.trim() - if (key) payload.llm_api_key = key - await updateSettings(payload) - setApiKey('') - if (key) setHasStoredKey(true) - toast.success(t('settings.aiParsing.toast.saved')) - } catch { - // updateSettings patches the store before the request and keeps the patch - // when the request fails, so a refused save (the 403 from #1772, or any - // other error) would leave the form showing a value the server never - // stored. Pull the stored settings back in so what is on screen is real. - await loadSettings() - toast.error(t('settings.aiParsing.toast.saveError')) - } finally { - setSaving(false) - } - } - return ( {t('settings.aiParsing.hint')} @@ -127,7 +75,7 @@ export default function MLlmConnectionSection(): React.ReactElement { trailing={ setMultimodal(v => !v)} + onChange={toggleMultimodal} ariaLabel={t('settings.aiParsing.multimodal')} /> }