From 0403c9418fd3a6cd246e44976901ac81f36a8651 Mon Sep 17 00:00:00 2001 From: dev_agent Date: Tue, 6 Oct 2026 10:13:52 -0400 Subject: [PATCH] Keep the stored-key removal path open and verify after sheet dismissal - The API key row stays visible whenever a key is stored, Ollama included, and Replace (with Remove key) stays enabled after a provider or base URL change, so "remove the key first" can be followed without undoing it. - One lock hint and one save-error guidance cover provider and base URL. - Closing the key sheet no longer resets the store mutation, so the PUT's verify still runs; a store error after dismissal shows under the key. - Reset and Save changes sit at the bottom of the screen (mock 1o). - Tests: base URL lock guidance, recover by removing the key with Ollama chosen, and dismissing the sheet while the PUT is pending. Co-Authored-By: Claude Opus 5.5 --- web/src/settings/AISettingsPage.module.css | 4 + web/src/settings/AISettingsPage.test.tsx | 90 +++++++++++++++++++++- web/src/settings/AISettingsPage.tsx | 71 +++++++++++------ 3 files changed, 136 insertions(+), 29 deletions(-) diff --git a/web/src/settings/AISettingsPage.module.css b/web/src/settings/AISettingsPage.module.css index e21e87d..a004dbe 100644 --- a/web/src/settings/AISettingsPage.module.css +++ b/web/src/settings/AISettingsPage.module.css @@ -1,5 +1,7 @@ /* Admin · AI assistant, handoff mock 1o. */ .page { + box-sizing: border-box; + min-height: 100%; max-width: 560px; display: flex; flex-direction: column; @@ -208,6 +210,8 @@ } .actions { + /* Mock 1o anchors Reset and Save changes at the bottom of the screen. */ + margin-top: auto; display: grid; grid-template-columns: minmax(0, 1fr) minmax(0, 2fr); gap: 10px; diff --git a/web/src/settings/AISettingsPage.test.tsx b/web/src/settings/AISettingsPage.test.tsx index 2ba43d8..4ddfc9f 100644 --- a/web/src/settings/AISettingsPage.test.tsx +++ b/web/src/settings/AISettingsPage.test.tsx @@ -19,7 +19,7 @@ const MODELS = { const DEFAULT_PROMPT = 'You are MealQuest, the default persona.' -type Responder = (body: unknown) => Response +type Responder = (body: unknown) => Response | Promise // stubAPI answers "METHOD path" from responders, each called with the // request's decoded JSON body; GET /settings/ai and /settings/ai/models @@ -180,13 +180,66 @@ describe('AISettingsPage', () => { }) await renderPage() fireEvent.change(provider(), { target: { value: 'openai' } }) - expect(screen.getByText(/Anthropic's API key is stored\. Remove it/)).toBeInTheDocument() - expect(screen.getByRole('button', { name: 'Replace' })).toBeDisabled() + expect(screen.getByText(/An API key is stored\. Remove it \(Replace → Remove key\)/)).toBeInTheDocument() + expect(screen.getByRole('button', { name: 'Replace' })).toBeEnabled() fireEvent.change(screen.getByRole('textbox', { name: 'Model id' }), { target: { value: 'gpt-5' } }) fireEvent.click(save()) const alert = await screen.findByRole('alert') expect(alert).toHaveTextContent('Not saved: invalid AI settings: provider: cannot change while an API key is stored') - expect(alert).toHaveTextContent('To switch provider, remove the API key first') + expect(alert).toHaveTextContent('Remove it (Replace → Remove key) before changing the provider or base URL.') + }) + + it('gives the same guidance when a stored key locks the base URL', async () => { + stubAPI({ + 'GET /api/v1/settings/ai': () => + jsonResponse(200, { ...AI_SETTINGS, provider: 'openai_compatible', base_url: 'https://llm.example.com/v1' }), + 'PATCH /api/v1/settings/ai': () => + jsonResponse(400, { + error: { + code: 'invalid_settings', + message: 'invalid AI settings: base_url: cannot change while an API key is stored: remove the key first', + retryable: false, + }, + }), + }) + await renderPage() + fireEvent.change(screen.getByRole('textbox', { name: 'Base URL' }), { target: { value: 'https://other.example.com/v1' } }) + expect(screen.getByText(/An API key is stored\. Remove it/)).toBeInTheDocument() + expect(screen.getByRole('button', { name: 'Replace' })).toBeEnabled() + fireEvent.click(save()) + expect(await screen.findByRole('alert')).toHaveTextContent( + 'Remove it (Replace → Remove key) before changing the provider or base URL.', + ) + }) + + it('recovers a provider switch by removing the stored key, without undoing the switch', async () => { + const fetchMock = stubAPI({ + 'PUT /api/v1/settings/ai/api-key': () => + jsonResponse(200, { ...AI_SETTINGS, api_key_last4: null, key_verified_at: null }), + 'PATCH /api/v1/settings/ai': (body) => + jsonResponse(200, { ...AI_SETTINGS, api_key_last4: null, key_verified_at: null, ...(body as object) }), + }) + await renderPage() + fireEvent.change(provider(), { target: { value: 'ollama' } }) + fireEvent.change(screen.getByRole('textbox', { name: 'Base URL' }), { target: { value: 'http://localhost:11434' } }) + fireEvent.change(screen.getByRole('textbox', { name: 'Model id' }), { target: { value: 'llama3.2' } }) + // The stored key stays in view, and removable, while Ollama is chosen. + expect(screen.getByText('••••••••3f9a')).toBeInTheDocument() + fireEvent.click(screen.getByRole('button', { name: 'Replace' })) + fireEvent.click(within(screen.getByRole('dialog')).getByRole('button', { name: 'Remove key' })) + + expect(await screen.findByText('Ollama takes no API key.')).toBeInTheDocument() + expect(screen.queryByText(/An API key is stored/)).not.toBeInTheDocument() + expect(provider()).toHaveValue('ollama') + fireEvent.click(save()) + expect(await screen.findByText('Changes saved.')).toBeInTheDocument() + expect(sent(fetchMock)).toEqual([ + { request: 'PUT /api/v1/settings/ai/api-key', body: { api_key: '' } }, + { + request: 'PATCH /api/v1/settings/ai', + body: { provider: 'ollama', base_url: 'http://localhost:11434', model: 'llama3.2' }, + }, + ]) }) it('replaces the key in a sheet, then verifies it', async () => { @@ -215,6 +268,35 @@ describe('AISettingsPage', () => { expect(document.body).not.toHaveTextContent('sk-ant-secret-b7c1') }) + it('still verifies the key when the sheet is dismissed while it is being stored', async () => { + let answerPut: (response: Response) => void = () => { + throw new Error('PUT not sent') + } + const fetchMock = stubAPI({ + 'PUT /api/v1/settings/ai/api-key': () => new Promise((resolve) => (answerPut = resolve)), + 'POST /api/v1/settings/ai/verify': () => + jsonResponse(200, { ...AI_SETTINGS, api_key_last4: 'b7c1', key_verified_at: '2026-10-06T14:00:00.000Z' }), + }) + await renderPage() + fireEvent.click(screen.getByRole('button', { name: 'Replace' })) + const sheet = screen.getByRole('dialog', { name: 'Replace API key' }) + fireEvent.change(within(sheet).getByLabelText('API key'), { target: { value: 'sk-ant-secret-b7c1' } }) + fireEvent.click(within(sheet).getByRole('button', { name: 'Save and verify' })) + await waitFor(() => expect(within(sheet).getByRole('button', { name: 'Saving…' })).toBeDisabled()) + fireEvent.keyDown(sheet, { key: 'Escape' }) + expect(screen.queryByRole('dialog')).not.toBeInTheDocument() + expect(screen.getByRole('button', { name: 'Saving…' })).toBeDisabled() + + act(() => answerPut(jsonResponse(200, { ...AI_SETTINGS, api_key_last4: 'b7c1', key_verified_at: null }))) + expect(await screen.findByText('Verified · Oct 6')).toBeInTheDocument() + expect(screen.getByText('••••••••b7c1')).toBeInTheDocument() + expect(screen.getByRole('button', { name: 'Replace' })).toBeEnabled() + expect(sent(fetchMock)).toEqual([ + { request: 'PUT /api/v1/settings/ai/api-key', body: { api_key: 'sk-ant-secret-b7c1' } }, + { request: 'POST /api/v1/settings/ai/verify', body: undefined }, + ]) + }) + it('shows the verify error and verifies again on request', async () => { let verifyCalls = 0 stubAPI({ diff --git a/web/src/settings/AISettingsPage.tsx b/web/src/settings/AISettingsPage.tsx index 404739d..3aecb69 100644 --- a/web/src/settings/AISettingsPage.tsx +++ b/web/src/settings/AISettingsPage.tsx @@ -104,6 +104,9 @@ function AISettingsForm({ saved }: { saved: AISettings }) { const patch = patchOf(saved, draft) const dirty = Object.keys(patch).length > 0 const providerChanged = draft.provider !== saved.provider + // The server keeps a stored key's provider and base URL fixed until the + // key is removed. + const keyLocked = saved.api_key_last4 !== null && (providerChanged || draft.base_url !== saved.base_url) const temperatureId = useId() // The model list is the saved provider's, so it is asked for only while // that provider is chosen and has the key it needs. @@ -200,12 +203,7 @@ function AISettingsForm({ saved }: { saved: AISettings }) { )} - {providerChanged && saved.api_key_last4 !== null && ( -

- {aiProviderLabels[saved.provider]}'s API key is stored. Remove it (Replace → Remove key) before switching - provider. -

- )} + {keyLocked &&

{keyLockedGuidance}

} {typedModel && ( edit({ base_url: event.target.value })} /> )} - {aiKeyPolicy[draft.provider] === 'none' ? ( -

{aiProviderLabels[draft.provider]} takes no API key.

- ) : ( - - )} +
@@ -318,7 +312,15 @@ export function formatVerifiedDate(timestamp: string): string { return new Intl.DateTimeFormat('en-US', { month: 'short', day: 'numeric' }).format(date) } -function APIKey({ saved, providerChanged }: { saved: AISettings; providerChanged: boolean }) { +// keyLockedGuidance is how to change the provider or base URL while a key +// is stored. +const keyLockedGuidance = + 'An API key is stored. Remove it (Replace → Remove key) before changing the provider or base URL.' + +// APIKey stays mounted whatever provider the draft names, so a key stored +// (and the verify that follows it) completes even if the household changes +// the provider meanwhile, and a stored key can always be removed. +function APIKey({ saved, draftProvider }: { saved: AISettings; draftProvider: AIProvider }) { const [replacing, setReplacing] = useState(false) const [key, setKey] = useState('') const store = useSetAIKey() @@ -326,11 +328,18 @@ function APIKey({ saved, providerChanged }: { saved: AISettings; providerChanged const labelId = useId() const sheetTitleId = useId() const last4 = saved.api_key_last4 + const providerChanged = draftProvider !== saved.provider + function open() { + store.reset() + setReplacing(true) + } + + // close keeps the store mutation: a key already sent is still stored and + // verified after the sheet is dismissed. function close() { setReplacing(false) setKey('') - store.reset() } function onStore() { @@ -348,6 +357,21 @@ function APIKey({ saved, providerChanged }: { saved: AISettings; providerChanged store.mutate('', { onSuccess: close }) } + const storeError = store.isError && ( +

+ The key was not stored: {store.error.message} +

+ ) + + if (last4 === null && aiKeyPolicy[draftProvider] === 'none') { + return ( +
+

{aiProviderLabels[draftProvider]} takes no API key.

+ {!replacing && storeError} +
+ ) + } + return (
@@ -358,13 +382,16 @@ function APIKey({ saved, providerChanged }: { saved: AISettings; providerChanged
- {providerChanged &&

Save the provider change before adding its key.

} + {last4 === null && providerChanged && ( +

Save the provider change before adding its key.

+ )} + {!replacing && storeError} @@ -391,11 +418,7 @@ function APIKey({ saved, providerChanged }: { saved: AISettings; providerChanged spellCheck={false} onChange={(event) => setKey(event.target.value)} /> - {store.isError && ( -

- The key was not stored: {store.error.message} -

- )} + {storeError}
{last4 !== null && (