feat(desktop): validate model provider settings

This commit is contained in:
DragonnZhang 2026-04-26 11:58:46 +08:00
parent 735f4cd8d2
commit 567ecca11a
9 changed files with 598 additions and 21 deletions

View file

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

View file

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

View file

@ -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"]');

View file

@ -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) => {

View file

@ -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 (
<section
@ -250,9 +261,20 @@ function ModelProvidersPanel({
</label>
<div className="settings-actions">
{validationReason ? (
<p
className="settings-validation error-text"
data-testid="settings-save-validation"
id="settings-save-validation"
>
{validationReason}
</p>
) : null}
<button
aria-describedby={saveValidationId}
className="primary-button"
disabled={state.loading || state.saving}
disabled={Boolean(saveDisabledReason)}
title={saveDisabledReason ?? 'Save model provider settings'}
type="button"
onClick={onSave}
>

View file

@ -16,6 +16,7 @@ import type {
DesktopProject,
DesktopSessionSummary,
DesktopTerminal,
DesktopUserSettings,
} from '../../api/client.js';
import { chatReducer, createInitialChatState } from '../../stores/chatStore.js';
import { createInitialModelState } from '../../stores/modelStore.js';
@ -500,6 +501,89 @@ describe('WorkspacePage', () => {
expect(onModelChange).toHaveBeenCalledWith('qwen-e2e-cdp');
});
it('shows inline settings validation before saving a model provider', () => {
const onSaveSettings = vi.fn();
const renderedContainer = renderWorkspace({
onSaveSettings,
settingsState: {
...createInitialSettingsState(),
settings: null,
form: {
provider: 'api-key',
apiKey: '',
codingPlanRegion: 'china',
activeModel: 'qwen-plus',
baseUrl: 'https://example.test/v1',
},
},
});
act(() => {
clickButton(renderedContainer, 'Settings');
});
const validation = renderedContainer.querySelector(
'[data-testid="settings-save-validation"]',
);
const saveButton = [...renderedContainer.querySelectorAll('button')].find(
(button) => button.textContent?.trim() === 'Save',
);
expect(validation?.textContent).toContain('Enter an API key');
expect(saveButton).toBeInstanceOf(HTMLButtonElement);
expect((saveButton as HTMLButtonElement).disabled).toBe(true);
expect(saveButton?.getAttribute('aria-describedby')).toBe(
'settings-save-validation',
);
act(() => {
(saveButton as HTMLButtonElement).click();
});
expect(onSaveSettings).not.toHaveBeenCalled();
});
it('allows settings save when a provider has a saved API key', () => {
const onSaveSettings = vi.fn();
const settings = createSettings();
const renderedContainer = renderWorkspace({
onSaveSettings,
settingsState: {
...createInitialSettingsState(),
settings,
form: {
provider: 'api-key',
apiKey: '',
codingPlanRegion: 'china',
activeModel: 'qwen-plus',
baseUrl: 'https://example.test/v1',
},
},
});
act(() => {
clickButton(renderedContainer, 'Settings');
});
const saveButton = [...renderedContainer.querySelectorAll('button')].find(
(button) => button.textContent?.trim() === 'Save',
);
expect(
renderedContainer.querySelector(
'[data-testid="settings-save-validation"]',
),
).toBeNull();
expect(saveButton).toBeInstanceOf(HTMLButtonElement);
expect((saveButton as HTMLButtonElement).disabled).toBe(false);
act(() => {
(saveButton as HTMLButtonElement).click();
});
expect(onSaveSettings).toHaveBeenCalledTimes(1);
});
it('bounds the inline changed-files summary before opening review', () => {
const manyFileDiff: DesktopGitDiff = {
...gitDiff,
@ -1153,3 +1237,29 @@ const readyLoadState: LoadState = {
state: 'ready',
status: readyStatus,
};
function createSettings(): DesktopUserSettings {
return {
ok: true,
settingsPath: '/tmp/settings.json',
provider: 'api-key',
selectedAuthType: 'openai',
model: { name: 'qwen-plus' },
codingPlan: {
region: 'china',
hasApiKey: false,
version: null,
},
openai: {
hasApiKey: true,
providers: [
{
id: 'qwen-plus',
name: 'Qwen Plus',
baseUrl: 'https://example.test/v1',
envKey: 'OPENAI_API_KEY',
},
],
},
};
}

View file

@ -9,6 +9,7 @@ import {
buildSettingsUpdateRequest,
createInitialSettingsState,
settingsReducer,
validateSettingsForm,
} from './settingsStore.js';
import type { DesktopUserSettings } from '../api/client.js';
@ -48,6 +49,93 @@ describe('settingsStore', () => {
},
});
});
it('trims API-key model settings before saving', () => {
const state = {
...createInitialSettingsState(),
form: {
provider: 'api-key' as const,
apiKey: ' secret ',
codingPlanRegion: 'china' as const,
activeModel: ' qwen-max ',
baseUrl: ' https://example.test/v1 ',
},
};
expect(buildSettingsUpdateRequest(state.form)).toEqual({
provider: 'api-key',
apiKey: 'secret',
activeModel: 'qwen-max',
modelProviders: {
'qwen-max': 'https://example.test/v1',
},
});
});
it('validates API-key provider inputs before saving', () => {
const state = createInitialSettingsState();
expect(
validateSettingsForm(
{ ...state.form, activeModel: ' ', apiKey: 'secret' },
null,
),
).toEqual({
valid: false,
reason: 'Enter a model name before saving.',
});
expect(
validateSettingsForm(
{ ...state.form, baseUrl: 'ftp://example.test', apiKey: 'secret' },
null,
),
).toEqual({
valid: false,
reason: 'Use a valid HTTP(S) base URL.',
});
expect(validateSettingsForm(state.form, null)).toEqual({
valid: false,
reason: 'Enter an API key to save this provider.',
});
expect(
validateSettingsForm({ ...state.form, apiKey: 'secret' }, null),
).toEqual({
valid: true,
reason: null,
});
});
it('accepts saved provider secrets without exposing them in the form', () => {
const settings = createSettings();
const state = settingsReducer(createInitialSettingsState(), {
type: 'load_success',
settings,
});
expect(state.form.apiKey).toBe('');
expect(validateSettingsForm(state.form, settings)).toEqual({
valid: true,
reason: null,
});
});
it('validates Coding Plan API keys before saving', () => {
const state = createInitialSettingsState();
const form = {
...state.form,
provider: 'coding-plan' as const,
apiKey: '',
};
expect(validateSettingsForm(form, null)).toEqual({
valid: false,
reason: 'Enter a Coding Plan API key to save this provider.',
});
expect(validateSettingsForm({ ...form, apiKey: 'secret' }, null)).toEqual({
valid: true,
reason: null,
});
});
});
function createSettings(): DesktopUserSettings {

View file

@ -25,6 +25,11 @@ export interface SettingsState {
error: string | null;
}
export interface SettingsFormValidation {
valid: boolean;
reason: string | null;
}
export type SettingsAction =
| { type: 'load_start' }
| { type: 'load_success'; settings: DesktopUserSettings }
@ -107,24 +112,77 @@ export function settingsReducer(
export function buildSettingsUpdateRequest(
form: SettingsFormState,
): UpdateDesktopSettingsRequest {
const apiKey = form.apiKey.trim() || undefined;
if (form.provider === 'coding-plan') {
return {
provider: 'coding-plan',
apiKey: form.apiKey || undefined,
apiKey,
codingPlanRegion: form.codingPlanRegion,
};
}
const activeModel = form.activeModel.trim();
const baseUrl = form.baseUrl.trim();
return {
provider: 'api-key',
apiKey: form.apiKey || undefined,
activeModel: form.activeModel,
apiKey,
activeModel,
modelProviders: {
[form.activeModel]: form.baseUrl,
[activeModel]: baseUrl,
},
};
}
export function validateSettingsForm(
form: SettingsFormState,
settings: DesktopUserSettings | null,
): SettingsFormValidation {
if (form.provider === 'coding-plan') {
if (
!hasIncomingOrSavedSecret(form.apiKey, settings?.codingPlan.hasApiKey)
) {
return {
valid: false,
reason: 'Enter a Coding Plan API key to save this provider.',
};
}
return { valid: true, reason: null };
}
if (form.activeModel.trim().length === 0) {
return {
valid: false,
reason: 'Enter a model name before saving.',
};
}
if (form.baseUrl.trim().length === 0) {
return {
valid: false,
reason: 'Enter an HTTP(S) base URL before saving.',
};
}
if (!isHttpBaseUrl(form.baseUrl)) {
return {
valid: false,
reason: 'Use a valid HTTP(S) base URL.',
};
}
if (!hasIncomingOrSavedSecret(form.apiKey, settings?.openai.hasApiKey)) {
return {
valid: false,
reason: 'Enter an API key to save this provider.',
};
}
return { valid: true, reason: null };
}
function formFromSettings(
settings: DesktopUserSettings,
current: SettingsFormState,
@ -144,3 +202,19 @@ function formFromSettings(
baseUrl: firstProvider?.baseUrl || current.baseUrl,
};
}
function hasIncomingOrSavedSecret(
incomingSecret: string,
hasSavedSecret: boolean | undefined,
): boolean {
return incomingSecret.trim().length > 0 || hasSavedSecret === true;
}
function isHttpBaseUrl(value: string): boolean {
try {
const url = new URL(value.trim());
return url.protocol === 'http:' || url.protocol === 'https:';
} catch {
return false;
}
}

View file

@ -2119,10 +2119,21 @@ textarea:focus {
.settings-actions {
display: flex;
align-items: center;
gap: 8px;
min-width: 0;
justify-content: flex-end;
}
.settings-validation {
min-width: 0;
flex: 1;
margin: 0;
overflow-wrap: anywhere;
font-size: 12px;
line-height: 1.35;
}
.settings-summary {
margin: 0;
overflow-wrap: anywhere;