From 567ecca11a3373b8c05993863710b38b92bb71ab Mon Sep 17 00:00:00 2001 From: DragonnZhang <731557579@qq.com> Date: Sun, 26 Apr 2026 11:58:46 +0800 Subject: [PATCH] feat(desktop): validate model provider settings --- .../model-configuration-workflow.md | 25 ++- ...de-electron-desktop-implementation-plan.md | 101 ++++++++++++ packages/desktop/scripts/e2e-cdp-smoke.mjs | 155 +++++++++++++++++- packages/desktop/src/renderer/App.tsx | 15 +- .../components/layout/SettingsPage.tsx | 32 +++- .../components/layout/WorkspacePage.test.tsx | 110 +++++++++++++ .../src/renderer/stores/settingsStore.test.ts | 88 ++++++++++ .../src/renderer/stores/settingsStore.ts | 82 ++++++++- packages/desktop/src/renderer/styles.css | 11 ++ 9 files changed, 598 insertions(+), 21 deletions(-) diff --git a/.qwen/e2e-tests/electron-desktop/model-configuration-workflow.md b/.qwen/e2e-tests/electron-desktop/model-configuration-workflow.md index 63a73998d..ce84dde7f 100644 --- a/.qwen/e2e-tests/electron-desktop/model-configuration-workflow.md +++ b/.qwen/e2e-tests/electron-desktop/model-configuration-workflow.md @@ -6,7 +6,7 @@ `cd packages/desktop && npm run e2e:cdp` - Result: pass - Artifact directory: - `.qwen/e2e-tests/electron-desktop/artifacts/2026-04-26T03-47-01-812Z/` + `.qwen/e2e-tests/electron-desktop/artifacts/2026-04-26T03-56-02-534Z/` ## Scenario @@ -15,17 +15,24 @@ 2. Open the fake project, type a first composer prompt, create a thread, and approve the fake command request. 3. Complete the existing review, discard-cancel safety, stage, and commit path. -4. Open Settings, save `qwen-e2e-cdp` with a fake API key and base URL, and - assert the saved model is visible without exposing the secret. -5. Return to Conversation and select `qwen-e2e-cdp` from the composer model +4. Open Settings, clear model/base URL/API key fields to assert inline + validation and disabled Save states, then enter `qwen-e2e-cdp` with a fake + API key and base URL. +5. Save the valid provider and assert the saved model is visible without + exposing the secret. +6. Return to Conversation and select `qwen-e2e-cdp` from the composer model picker. -6. Continue through terminal expand, command execution, attach-to-composer, and +7. Continue through terminal expand, command execution, attach-to-composer, and send verification. ## Assertions - The Settings default view includes product sections and keeps diagnostics behind Advanced. +- The Settings model provider form disables Save with inline reasons for an + empty model, invalid base URL, and missing API key. +- The validation row stays contained in the model card and does not create body + overflow. - The API key input remains `type="password"` and is cleared after save. - The fake API key is absent from settings text, composer text, input values, advanced diagnostics, and the model-switch artifact. @@ -42,6 +49,7 @@ ## Artifacts - `settings-page.png` +- `settings-validation.json` - `settings-product-state.json` - `settings-advanced-diagnostics.json` - `composer-model-switch.json` @@ -50,6 +58,7 @@ ## Known Uncovered Risk -The harness verifies the API-key model save and active-session model switch -using fake ACP. It does not yet cover Coding Plan model switching, invalid API -key validation, or keyboard-only navigation through the model picker. +The harness verifies the API-key model validation/save and active-session model +switch using fake ACP. It does not yet cover Coding Plan model switching, +server-side invalid API key rejection, or keyboard-only navigation through the +model picker. diff --git a/design/qwen-code-electron-desktop-implementation-plan.md b/design/qwen-code-electron-desktop-implementation-plan.md index 992fbcb40..4aed0056c 100644 --- a/design/qwen-code-electron-desktop-implementation-plan.md +++ b/design/qwen-code-electron-desktop-implementation-plan.md @@ -22,6 +22,107 @@ execution order, verification, decisions, and remaining work. ## Codex Alignment Progress +### Completed Slice: Settings Model Validation Feedback + +Status: completed in iteration 28. + +Goal: make model provider setup fail early in the Settings surface with compact +inline validation, so users understand why Save is unavailable before a request +hits the desktop server. + +User-visible value: users adding an API-key model configuration see specific, +contained reasons for missing model, invalid base URL, or missing API key +states. The Save action only enables when the active provider form is valid, +and saved secrets still never render back into the DOM. + +Expected files: + +- `packages/desktop/src/renderer/stores/settingsStore.ts` +- `packages/desktop/src/renderer/stores/settingsStore.test.ts` +- `packages/desktop/src/renderer/components/layout/SettingsPage.tsx` +- `packages/desktop/src/renderer/components/layout/WorkspacePage.test.tsx` +- `packages/desktop/src/renderer/styles.css` +- `packages/desktop/scripts/e2e-cdp-smoke.mjs` +- `.qwen/e2e-tests/electron-desktop/model-configuration-workflow.md` +- `design/qwen-code-electron-desktop-implementation-plan.md` + +Acceptance criteria: + +- API-key settings trim model and base URL before saving. +- Save is disabled with a clear inline reason when the API-key provider has no + model, an invalid HTTP(S) base URL, or no new/saved API key. +- Coding Plan settings keep the region control compact and require a new or + saved API key before Save enables. +- The validation message is contained inside the settings card and does not + expose typed or saved secrets. +- Existing successful model save and composer model-switch behavior continues + through the real Electron CDP path. + +Verification: + +- Unit/component test command: + `cd packages/desktop && SHELL=/bin/bash npx vitest run src/renderer/stores/settingsStore.test.ts src/renderer/components/layout/WorkspacePage.test.tsx` +- Syntax command: `node --check packages/desktop/scripts/e2e-cdp-smoke.mjs` +- Build/typecheck/lint commands: + `cd packages/desktop && npm run typecheck && npm run lint && npm run build` +- Real Electron harness: + `cd packages/desktop && npm run e2e:cdp` +- Harness path: `packages/desktop/scripts/e2e-cdp-smoke.mjs` +- E2E scenario steps: launch real Electron with isolated HOME/runtime/user-data + and fake ACP, complete the existing composer, review, and commit path, open + Settings, clear model/base URL/API key fields to assert inline validation and + disabled Save states, then enter a valid fake model/base URL/API key, save, + return to Conversation, and switch to the saved model from the composer. +- E2E assertions: invalid settings states keep Save disabled with visible + reason text, the validation card stays within Settings without body overflow, + valid input re-enables Save, the fake API key is absent from settings text and + artifacts, and no console errors or failed local requests are recorded. +- Diagnostic artifacts: `settings-validation.json`, + `settings-product-state.json`, `composer-model-switch.json`, + `settings-page.png`, Electron log, and summary JSON under + `.qwen/e2e-tests/electron-desktop/artifacts/`. +- Required skills applied: `frontend-design` for prototype-constrained inline + validation that keeps Settings dense and readable; `electron-desktop-dev` for + renderer changes and real Electron CDP verification. + +Notes and decisions: + +- This slice keeps Settings as the existing full workbench page. Converting it + to a drawer or modal is a separate fidelity pass. +- Validation stays client-side for immediate feedback while the desktop server + remains the authority for persisted settings and secret handling. +- Save calls are guarded in `App` as well as disabled in the form, so a stale + or programmatic click still gets the same validation message instead of + sending an incomplete request. +- API-key model and base URL values are trimmed before the update request is + built. The API key input is also trimmed when sent, but saved secrets are + still cleared from the form after persistence. + +Verification results: + +- `node --check packages/desktop/scripts/e2e-cdp-smoke.mjs` passed. +- `git diff --check` passed. +- `cd packages/desktop && SHELL=/bin/bash npx vitest run src/renderer/stores/settingsStore.test.ts src/renderer/components/layout/WorkspacePage.test.tsx` + passed with 24 tests. +- `cd packages/desktop && npm run typecheck` passed. +- `cd packages/desktop && npm run lint` passed with no warnings after the + dependency fix. +- `cd packages/desktop && npm run build` passed. +- `cd packages/desktop && npm run e2e:cdp` passed through real Electron at + `.qwen/e2e-tests/electron-desktop/artifacts/2026-04-26T03-56-02-534Z/`. +- Key recorded validation metrics: missing model, invalid base URL, and missing + API key each disabled Save with inline reasons; valid input enabled Save; + the validation row stayed inside the 500 px model card; no document overflow; + fake API key visible text exposure was false; console errors and failed local + requests were both zero. + +Next work: + +- Continue model configuration by adding keyboard-focused coverage for the + provider selector and Coding Plan path. +- Continue prototype fidelity by exploring whether Settings should open as a + narrower supporting surface instead of replacing the full workbench. + ### Completed Slice: Composer Model Provider Promotion Status: completed in iteration 27. diff --git a/packages/desktop/scripts/e2e-cdp-smoke.mjs b/packages/desktop/scripts/e2e-cdp-smoke.mjs index 87989e212..3c4949ae9 100644 --- a/packages/desktop/scripts/e2e-cdp-smoke.mjs +++ b/packages/desktop/scripts/e2e-cdp-smoke.mjs @@ -176,9 +176,7 @@ async function main() { await waitForSelector('[data-testid="settings-page"]'); await assertSettingsPageLayout('settings-layout.json'); await saveScreenshot('settings-page.png'); - await setFieldByLabel('Model', 'qwen-e2e-cdp'); - await setFieldByLabel('Base URL', 'https://example.invalid/v1'); - await setFieldByLabel('API key', 'sk-desktop-e2e'); + await assertSettingsValidation('settings-validation.json'); await clickButton('Save'); await waitForText('qwen-e2e-cdp'); await assertSettingsProductState('settings-product-state.json'); @@ -4058,6 +4056,157 @@ async function assertSettingsPageLayout(fileName) { } } +async function assertSettingsValidation(fileName) { + const snapshots = {}; + + await setFieldByLabel('Model', ''); + await waitForText('Enter a model name before saving.'); + snapshots.missingModel = await readSettingsValidationSnapshot(); + + await setFieldByLabel('Model', 'qwen-e2e-cdp'); + await setFieldByLabel('Base URL', 'not-a-url'); + await waitForText('Use a valid HTTP(S) base URL.'); + snapshots.invalidBaseUrl = await readSettingsValidationSnapshot(); + + await setFieldByLabel('Base URL', 'https://example.invalid/v1'); + await setFieldByLabel('API key', ''); + await waitForText('Enter an API key to save this provider.'); + snapshots.missingApiKey = await readSettingsValidationSnapshot(); + + await setFieldByLabel('API key', 'sk-desktop-e2e'); + await waitFor( + 'valid settings save enabled', + async () => { + const snapshot = await readSettingsValidationSnapshot(); + return ( + snapshot.saveDisabled === false && + snapshot.validationText === '' && + snapshot.apiKeyLength > 0 + ); + }, + 5_000, + ); + snapshots.valid = await readSettingsValidationSnapshot(); + + await writeFile( + join(artifactDir, fileName), + `${JSON.stringify(snapshots, null, 2)}\n`, + 'utf8', + ); + + if ( + snapshots.missingModel.validationText !== + 'Enter a model name before saving.' + ) { + throw new Error( + `Missing-model validation did not render: ${JSON.stringify( + snapshots.missingModel, + )}`, + ); + } + + if ( + snapshots.invalidBaseUrl.validationText !== + 'Use a valid HTTP(S) base URL.' + ) { + throw new Error( + `Invalid-base-URL validation did not render: ${JSON.stringify( + snapshots.invalidBaseUrl, + )}`, + ); + } + + if ( + snapshots.missingApiKey.validationText !== + 'Enter an API key to save this provider.' + ) { + throw new Error( + `Missing-API-key validation did not render: ${JSON.stringify( + snapshots.missingApiKey, + )}`, + ); + } + + for (const [name, snapshot] of Object.entries(snapshots)) { + const shouldBeDisabled = name !== 'valid'; + if (snapshot.saveDisabled !== shouldBeDisabled) { + throw new Error( + `Unexpected Save disabled state for ${name}: ${JSON.stringify( + snapshot, + )}`, + ); + } + + if (snapshot.hasSecretText) { + throw new Error( + `Settings validation exposed the fake API key in visible text for ${name}.`, + ); + } + + if (snapshot.validationOverflow || snapshot.documentOverflow) { + throw new Error( + `Settings validation overflowed for ${name}: ${JSON.stringify( + snapshot, + )}`, + ); + } + } +} + +async function readSettingsValidationSnapshot() { + return evaluate(`(() => { + const settings = document.querySelector('[data-testid="settings-page"]'); + const modelConfig = document.querySelector('[data-testid="model-config"]'); + const validation = document.querySelector( + '[data-testid="settings-save-validation"]' + ); + const saveButton = [...document.querySelectorAll('button')].find( + (button) => button.textContent.trim() === 'Save' + ); + const apiKey = [...document.querySelectorAll('label')] + .find((candidate) => + candidate.innerText.trim().toLowerCase().startsWith('api key') + ) + ?.querySelector('input'); + const rectFor = (element) => { + if (!element) { + return null; + } + const rect = element.getBoundingClientRect(); + return { + top: rect.top, + right: rect.right, + bottom: rect.bottom, + left: rect.left, + width: rect.width, + height: rect.height + }; + }; + const settingsRect = rectFor(settings); + const modelConfigRect = rectFor(modelConfig); + const validationRect = rectFor(validation); + return { + validationText: validation?.textContent.trim() ?? '', + saveDisabled: saveButton?.disabled ?? null, + saveDescribedBy: saveButton?.getAttribute('aria-describedby') ?? null, + apiKeyType: apiKey?.getAttribute('type') ?? null, + apiKeyLength: apiKey?.value.length ?? 0, + hasSecretText: (settings?.innerText ?? '').includes('sk-desktop-e2e'), + validationOverflow: + validationRect !== null && + modelConfigRect !== null && + (validationRect.left < modelConfigRect.left - 1 || + validationRect.right > modelConfigRect.right + 1), + documentOverflow: + document.body.scrollWidth > window.innerWidth + 4 || + document.body.scrollHeight > window.innerHeight + 4, + settingsWidth: settingsRect?.width ?? null, + modelConfigWidth: modelConfigRect?.width ?? null, + validationWidth: validationRect?.width ?? null + }; + })()`); +} + async function assertSettingsProductState(fileName) { const snapshot = await evaluate(`(() => { const settings = document.querySelector('[data-testid="settings-page"]'); diff --git a/packages/desktop/src/renderer/App.tsx b/packages/desktop/src/renderer/App.tsx index 8d299453c..5780b887e 100644 --- a/packages/desktop/src/renderer/App.tsx +++ b/packages/desktop/src/renderer/App.tsx @@ -65,6 +65,7 @@ import { buildSettingsUpdateRequest, createInitialSettingsState, settingsReducer, + validateSettingsForm, } from './stores/settingsStore.js'; import { WorkspacePage } from './components/layout/WorkspacePage.js'; import type { LoadState } from './components/layout/types.js'; @@ -844,6 +845,18 @@ export function App() { return; } + const validation = validateSettingsForm( + settingsState.form, + settingsState.settings, + ); + if (!validation.valid) { + dispatchSettings({ + type: 'save_error', + message: validation.reason ?? 'Settings are incomplete.', + }); + return; + } + dispatchSettings({ type: 'save_start' }); try { const settings = await updateDesktopUserSettings( @@ -855,7 +868,7 @@ export function App() { } catch (error) { dispatchSettings({ type: 'save_error', message: getErrorMessage(error) }); } - }, [loadState, settingsState.form]); + }, [loadState, settingsState.form, settingsState.settings]); const authenticate = useCallback( async (methodId: string) => { diff --git a/packages/desktop/src/renderer/components/layout/SettingsPage.tsx b/packages/desktop/src/renderer/components/layout/SettingsPage.tsx index 12c0f8dad..dd64ce043 100644 --- a/packages/desktop/src/renderer/components/layout/SettingsPage.tsx +++ b/packages/desktop/src/renderer/components/layout/SettingsPage.tsx @@ -7,10 +7,11 @@ import { useState, type Dispatch } from 'react'; import type { ChatState } from '../../stores/chatStore.js'; import type { ModelState } from '../../stores/modelStore.js'; -import type { - SettingsAction, - SettingsFormState, - SettingsState, +import { + validateSettingsForm, + type SettingsAction, + type SettingsFormState, + type SettingsState, } from '../../stores/settingsStore.js'; import type { DesktopApprovalMode } from '../../../shared/desktopProtocol.js'; import type { LoadState } from './types.js'; @@ -154,6 +155,16 @@ function ModelProvidersPanel({ state: SettingsState; }) { const provider = state.form.provider; + const validation = validateSettingsForm(state.form, state.settings); + const validationReason = validation.valid ? null : validation.reason; + const saveDisabledReason = state.loading + ? 'Settings are still loading.' + : state.saving + ? 'Saving settings.' + : validationReason; + const saveValidationId = validationReason + ? 'settings-save-validation' + : undefined; return (
+ {validationReason ? ( +

+ {validationReason} +

+ ) : null}