feat(desktop): implement settings tabs with multi-provider management#16
feat(desktop): implement settings tabs with multi-provider management#16
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Findings
-
[Major] Re-adding a provider after deleting the last saved key keeps a ghost active provider and stale model IDs —
settings:delete-providerwritesprovider: 'openai'when no secrets remain, andsettings:add-providerreuses any supportedcachedConfig.providerwithout checking that the active provider still has a saved secret. After deleting the last provider, adding a new one leaves it inactive and onboarding still incomplete, evidenceapps/desktop/src/main/onboarding-ipc.ts:243,apps/desktop/src/main/onboarding-ipc.ts:275.
Suggested fix:const current = cachedConfig; const hasActiveSecret = current !== null && current.secrets[current.provider] !== undefined; const activeProvider = hasActiveSecret ? current.provider : input.provider; const modelPrimary = hasActiveSecret ? current.modelPrimary : input.modelPrimary; const modelFast = hasActiveSecret ? current.modelFast : input.modelFast;
-
[Major] Decryption failures are silently masked as
***— the new provider listing catchesdecryptSecreterrors and substitutes an empty string, which hides keychain /safeStoragebreakage instead of surfacing it, evidenceapps/desktop/src/main/onboarding-ipc.ts:112.
Suggested fix:try { plain = decryptSecret(ref.ciphertext); } catch (err) { throw new CodesignError(`Failed to decrypt key for ${p}`, 'SECRET_DECRYPT_FAILED', { cause: err, }); }
-
[Major] The new Language / Update channel / Generation timeout controls silently discard user input — the language select is hard-coded to
value="en"with a no-op handler, and the advanced controls only mutate component state. There is no matching preload/main/config persistence path, so values reset on reopen, evidenceapps/desktop/src/renderer/src/components/Settings.tsx:705,apps/desktop/src/renderer/src/components/Settings.tsx:854,packages/shared/src/config.ts:30.
Suggested fix:const [locale, setLocale] = useState(currentLocale); <NativeSelect value={locale} onChange={(next) => { setLocale(next); void window.codesign.settings.setLocale(next); }} />
Or, until the persistence path exists, render these controls disabled with a clear "Coming soon" label.
-
[Major] “Open” failures are silent on both IPC and UI sides —
shell.openPathreturns an error string that must be checked, butsettings:open-folderignores it and the renderer fire-and-forgets the promise. Bad paths or OS failures give the user no feedback, evidenceapps/desktop/src/main/onboarding-ipc.ts:335,apps/desktop/src/renderer/src/components/Settings.tsx:774.
Suggested fix:const error = await shell.openPath(raw); if (error) { throw new CodesignError(`Could not open ${raw}: ${error}`, 'OPEN_PATH_FAILED'); }
Summary
- Review mode: initial
- 4 issues found in the new settings flows: one provider-state regression, two silent-failure paths, and one group of settings controls that currently discard user input.
Testing
- Not run (automation).
- Not found in repo/docs: Vitest or Playwright coverage for the new
settings:*IPC paths or the newSettingsUI flows.
open-codesign Bot
| delete nextBaseUrls[input.provider]; | ||
| } | ||
| // When adding the first provider, make it active. | ||
| const activeProvider = |
There was a problem hiding this comment.
Deleting the last provider leaves cachedConfig.provider set to a supported string (openai) even though there is no matching secret, so this branch reuses a ghost active provider and stale model IDs on the next add. The first provider added after that stays inactive and onboarding remains blocked.
Suggested fix:
const current = cachedConfig;
const hasActiveSecret =
current !== null && current.secrets[current.provider] !== undefined;
const activeProvider = hasActiveSecret ? current.provider : input.provider;
const modelPrimary = hasActiveSecret ? current.modelPrimary : input.modelPrimary;
const modelFast = hasActiveSecret ? current.modelFast : input.modelFast;| for (const [p, ref] of Object.entries(cfg.secrets)) { | ||
| if (!isSupportedOnboardingProvider(p) || ref === undefined) continue; | ||
| let plain: string; | ||
| try { |
There was a problem hiding this comment.
This catch turns keychain / safeStorage failures into a fake masked key (***), which hides the real problem from the user and violates the repo's “no silent fallbacks” rule.
Suggested fix:
try {
plain = decryptSecret(ref.ciphertext);
} catch (err) {
throw new CodesignError(`Failed to decrypt key for ${p}`, 'SECRET_DECRYPT_FAILED', {
cause: err,
});
}| <div className="pt-2 border-t border-[var(--color-border-subtle)]"> | ||
| <Row label="Language" hint="UI language. Affects text only, not generation."> | ||
| <NativeSelect | ||
| value="en" |
There was a problem hiding this comment.
This select is hard-coded to en and has a no-op onChange. The new advanced controls below are also local component state only, so the settings panel currently discards user input on close/reopen.
Suggested fix:
<NativeSelect
value={locale}
onChange={(next) => {
setLocale(next);
void window.codesign.settings.setLocale(next);
}}
/>Or keep the controls disabled until the persistence path exists.
| data: configDir(), | ||
| })); | ||
|
|
||
| ipcMain.handle('settings:open-folder', async (_e, raw: unknown) => { |
There was a problem hiding this comment.
openPath() does not throw on failure; it returns an error string. Ignoring that here, and then fire-and-forgetting the renderer call, makes path-open failures invisible to the user.
Suggested fix:
const error = await shell.openPath(raw);
if (error) {
throw new CodesignError(`Could not open ${raw}: ${error}`, 'OPEN_PATH_FAILED');
}
|
目前不建议合并，先处理下面这些问题：
这个 PR 改动面比较大，建议先把上面几项行为和持久化问题收敛完，再考虑合并。 |
There was a problem hiding this comment.
Findings
-
[Major] Debounced model saves outlive the active row and can switch the provider back after the user already moved on.
ActiveModelSelectorschedulessettings:set-active-provider400 ms later, but the timer is never cleared on unmount. If the user changes a model and immediately clicks "Set active" on another provider, the stale timeout from the old row still fires and re-activates the previous provider, evidenceapps/desktop/src/renderer/src/components/Settings.tsx:516,apps/desktop/src/renderer/src/components/Settings.tsx:539,apps/desktop/src/renderer/src/components/Settings.tsx:545.
Suggested fix:useEffect(() => { return () => { if (saveTimeout.current !== null) clearTimeout(saveTimeout.current); }; }, []);
-
[Minor] The Storage tab's "Open" action does not open folders for the Config and Logs rows.
settings:get-pathsreturnsconfigPath()andgetLogPath(), which are file paths, andsettings:open-folderpasses them directly toshell.openPath(). That opens the file / associated app instead of the containing folder for two of the three rows, evidenceapps/desktop/src/main/onboarding-ipc.ts:297,apps/desktop/src/main/onboarding-ipc.ts:298, contextapps/desktop/src/main/config.ts:16,apps/desktop/src/main/logger.ts:62.
Suggested fix:import { dirname } from 'node:path'; ipcMain.handle('settings:get-paths', () => ({ config: configPath(), configFolder: dirname(configPath()), logs: getLogPath(), logsFolder: dirname(getLogPath()), data: configDir(), }));
Then wire the Open buttons to the
*Folderfields.
Questions
- Can you add a
Signed-off-bytrailer to commit7109ffb194be283a1308db514ce296edaed042c0? The follow-up commit is signed, but the original feature commit is not.
Summary
- Review mode: follow-up after new commits
- 2 issues remain on the current head: the debounced active-model save can revert a later provider switch, and the Storage tab still opens files instead of folders for Config/Logs.
Testing
- Not run (automation).
- Not found in repo/docs: coverage for the
ActiveModelSelectorunmount/debounce path or the Storage tab's folder-open behavior; the new Vitest coverage only exercisesapps/desktop/src/main/provider-settings.test.ts.
open-codesign Bot
| function handlePrimaryChange(v: string) { | ||
| setPrimary(v); | ||
| if (saveTimeout.current !== null) clearTimeout(saveTimeout.current); | ||
| saveTimeout.current = setTimeout(() => void save(v, fast), 400); |
There was a problem hiding this comment.
This timer is never cleared on unmount. If a user changes a model and immediately activates another provider, the stale timeout from this row still fires 400 ms later and calls settings:set-active-provider for the old provider, flipping the active selection back.
Suggested fix:
useEffect(() => {
return () => {
if (saveTimeout.current !== null) clearTimeout(saveTimeout.current);
};
}, []);| if (typeof raw !== 'string') { | ||
| throw new CodesignError('open-folder expects a path string', 'IPC_BAD_INPUT'); | ||
| } | ||
| const error = await shell.openPath(raw); |
There was a problem hiding this comment.
configPath() and getLogPath() are file paths, so passing them straight to shell.openPath() opens the file / associated app instead of the containing folder promised by this settings action.
Suggested fix:
import { dirname } from 'node:path';
ipcMain.handle('settings:get-paths', () => ({
config: configPath(),
configFolder: dirname(configPath()),
logs: getLogPath(),
logsFolder: dirname(getLogPath()),
data: configDir(),
}));Then use the *Folder fields for the Open buttons.
There was a problem hiding this comment.
Findings
-
[Major] Add-provider validation can be skipped entirely —
canSavebecomes true for any non-empty API key, so the modal allows an unvalidated key to be persisted and then shows a success toast. That leaves the first saved provider able to become active with credentials the UI never actually verified, evidenceapps/desktop/src/renderer/src/components/Settings.tsx:268,apps/desktop/src/renderer/src/components/Settings.tsx:395.
Suggested fix:const canSave = form.apiKey.trim().length > 0 && form.validated && !form.validating;
-
[Minor]
settings:set-active-providerstill accepts providers with no stored key — the handler validates the string shape but never checkscfg.secrets[provider]before writing the config, so stale or malformed renderer input can persist an active provider that cannot generate, evidenceapps/desktop/src/main/onboarding-ipc.ts:270,apps/desktop/src/main/onboarding-ipc.ts:283.
Suggested fix:if (cfg.secrets[provider] === undefined) { throw new CodesignError( `No API key stored for provider "${provider}".`, 'PROVIDER_KEY_MISSING', ); }
Questions
- Can you add a
Signed-off-bytrailer to commit7109ffb194be283a1308db514ce296edaed042c0? The two follow-up commits are signed, but the original feature commit still is not.
Summary
- Review mode: follow-up after new commits
- 2 issues remain on the current head: the add-provider flow still bypasses validation, and the main-process activation handler can still persist an active provider with no saved secret.
- No new dependency, license, or direct provider SDK import issues found in this diff.
Testing
- Not run (automation).
- Missing coverage for the add-provider validation gate in
Settings.tsxand for rejecting unsaved providers inonboarding-ipc.ts.
open-codesign Bot
| const fastOptions = sl.fast.map((m) => ({ value: m, label: m })); | ||
| const canSave = | ||
| form.apiKey.trim().length > 0 && | ||
| (form.validated || form.baseUrl.trim().length > 0 || form.apiKey.trim().length > 0); |
There was a problem hiding this comment.
canSave is effectively just form.apiKey.trim().length > 0, because the final || form.apiKey.trim().length > 0 makes validation optional. That means an invalid or never-validated key can still be saved and marked as successful.
Suggested fix:
const canSave = form.apiKey.trim().length > 0 && form.validated && !form.validating;| if (cfg === null) { | ||
| throw new CodesignError('No configuration found', 'CONFIG_MISSING'); | ||
| } | ||
| const next: Config = { |
There was a problem hiding this comment.
This handler still writes provider into config without proving a key exists for that provider. If the renderer sends a stale or malformed selection, the app can persist an active provider that immediately fails generation.
Suggested fix:
if (cfg.secrets[provider] === undefined) {
throw new CodesignError(
`No API key stored for provider "${provider}".`,
'PROVIDER_KEY_MISSING',
);
}
There was a problem hiding this comment.
Findings
-
[Major] Deleting the active provider can leave the next active provider with the deleted provider's model IDs, so the app can persist an invalid provider/model pair after a delete, evidence
apps/desktop/src/main/onboarding-ipc.ts:244,apps/desktop/src/main/onboarding-ipc.ts:251.
Suggested fix:const nextActive = cfg.provider === raw ? (remaining[0] ?? 'openai') : cfg.provider; const defaults = cfg.provider === raw ? PROVIDER_SHORTLIST[nextActive] : null; const next: Config = { version: 1, provider: nextActive, modelPrimary: defaults?.defaultPrimary ?? cfg.modelPrimary, modelFast: defaults?.defaultFast ?? cfg.modelFast, secrets: nextSecrets, baseUrls: nextBaseUrls, };
-
[Major] An in-flight validation response can mark edited credentials as validated.
setFieldclearsvalidated, buthandleValidateunconditionally sets it back totruewhen the old request resolves, so a different API key or base URL can be saved without ever being checked, evidenceapps/desktop/src/renderer/src/components/Settings.tsx:224,apps/desktop/src/renderer/src/components/Settings.tsx:243.
Suggested fix:const snapshot = { provider: form.provider, apiKey: form.apiKey.trim(), baseUrl: form.baseUrl.trim(), }; const res = await window.codesign.settings.validateKey({ provider: snapshot.provider, apiKey: snapshot.apiKey, ...(snapshot.baseUrl ? { baseUrl: snapshot.baseUrl } : {}), }); setForm((prev) => { const stale = prev.provider !== snapshot.provider || prev.apiKey.trim() !== snapshot.apiKey || prev.baseUrl.trim() !== snapshot.baseUrl; if (stale) return { ...prev, validating: false }; return res.ok ? { ...prev, validating: false, validated: true } : { ...prev, validating: false, validated: false, error: res.message }; });
-
[Major] The new settings IPC surface is unversioned, which violates the repo's compatibility rule for every IPC contract, evidence
docs/PRINCIPLES.md:75,apps/desktop/src/main/onboarding-ipc.ts:202,apps/desktop/src/preload/index.ts:79.
Suggested fix:ipcMain.handle('settings:v1:list-providers', (): ProviderRow[] => { return toProviderRows(getCachedConfig(), decryptSecret); }); listProviders: () => ipcRenderer.invoke('settings:v1:list-providers') as Promise<ProviderRow[]>,
Questions
- Can you add a
Signed-off-bytrailer to commit7109ffb194be283a1308db514ce296edaed042c0? The follow-up fix commits are signed, but the original feature commit in this PR still is not.
Summary
- Review mode: follow-up after new commits
- 3 issues remain on the current head: deleting the active provider can persist invalid model IDs, validation can go stale across edits, and the new settings IPC channels still ship without versioning.
- No new dependency, license, or direct provider SDK import issues found in this diff.
Testing
- Not run (automation).
- Missing coverage for deleting the active provider and for stale validation responses while the form is being edited.
open-codesign Bot
| const next: Config = { | ||
| version: 1, | ||
| provider: nextActive, | ||
| modelPrimary: cfg.modelPrimary, |
There was a problem hiding this comment.
If raw is the active provider, this writes nextActive but keeps cfg.modelPrimary / cfg.modelFast from the deleted provider. Deleting OpenAI while Anthropic remains can therefore persist provider = "anthropic" with modelPrimary = "gpt-4o", which breaks the next generation request.
Suggested fix:
const defaults = cfg.provider === raw ? PROVIDER_SHORTLIST[nextActive] : null;
const next: Config = {
version: 1,
provider: nextActive,
modelPrimary: defaults?.defaultPrimary ?? cfg.modelPrimary,
modelFast: defaults?.defaultFast ?? cfg.modelFast,
secrets: nextSecrets,
baseUrls: nextBaseUrls,
};| ...(trimmedUrl.length > 0 ? { baseUrl: trimmedUrl } : {}), | ||
| }); | ||
| if (res.ok) { | ||
| setForm((prev) => ({ ...prev, validating: false, validated: true })); |
There was a problem hiding this comment.
setField clears validated, but this async completion marks the current form as validated even if the user changed provider / API key / base URL while the request was in flight. That re-enables Save for credentials that were never actually checked.
Suggested fix:
const snapshot = {
provider: form.provider,
apiKey: form.apiKey.trim(),
baseUrl: form.baseUrl.trim(),
};
const res = await window.codesign.settings.validateKey({
provider: snapshot.provider,
apiKey: snapshot.apiKey,
...(snapshot.baseUrl ? { baseUrl: snapshot.baseUrl } : {}),
});
setForm((prev) => {
const stale =
prev.provider !== snapshot.provider ||
prev.apiKey.trim() !== snapshot.apiKey ||
prev.baseUrl.trim() !== snapshot.baseUrl;
if (stale) return { ...prev, validating: false };
return res.ok
? { ...prev, validating: false, validated: true }
: { ...prev, validating: false, validated: false, error: res.message };
});|
|
||
| // ── Settings: provider management ────────────────────────────────────────── | ||
|
|
||
| ipcMain.handle('settings:list-providers', (): ProviderRow[] => { |
There was a problem hiding this comment.
These new settings:* channels are introduced without a version namespace, but docs/PRINCIPLES.md requires every IPC contract to be versioned. Shipping them unversioned now makes the first future rename or payload change a breaking change.
Suggested fix:
ipcMain.handle('settings:v1:list-providers', (): ProviderRow[] => {
return toProviderRows(getCachedConfig(), decryptSecret);
});
listProviders: () =>
ipcRenderer.invoke('settings:v1:list-providers') as Promise<ProviderRow[]>,
hqhq1025
force-pushed
the
wt/settings-real
branch
from
3ed3144 to
2412e13
Compare
There was a problem hiding this comment.
Findings
-
[Blocker] Deleting the active provider can persist an invalid provider/model pair — when
settings:delete-providerswitchesprovider, it still writes the deleted provider'smodelPrimary/modelFast, which can break the next generation request (apps/desktop/src/main/onboarding-ipc.ts:270,apps/desktop/src/main/onboarding-ipc.ts:271).
Suggested fix:const switchedActive = cfg.provider === raw; const defaults = switchedActive ? PROVIDER_SHORTLIST[nextActive] : null; const next: Config = { version: 1, provider: nextActive, modelPrimary: defaults?.defaultPrimary ?? cfg.modelPrimary, modelFast: defaults?.defaultFast ?? cfg.modelFast, secrets: nextSecrets, baseUrls: nextBaseUrls, };
-
[Major] Async key validation can incorrectly mark edited credentials as validated —
handleValidatesetsvalidated: truewhen the request resolves, even if provider/key/base URL changed while it was in flight (apps/desktop/src/renderer/src/components/Settings.tsx:228,apps/desktop/src/renderer/src/components/Settings.tsx:238).
Suggested fix:const snapshot = { provider: form.provider, apiKey: form.apiKey.trim(), baseUrl: form.baseUrl.trim(), }; const res = await window.codesign.settings.validateKey({ provider: snapshot.provider, apiKey: snapshot.apiKey, ...(snapshot.baseUrl ? { baseUrl: snapshot.baseUrl } : {}), }); setForm((prev) => { const stale = prev.provider !== snapshot.provider || prev.apiKey.trim() !== snapshot.apiKey || prev.baseUrl.trim() !== snapshot.baseUrl; if (stale) return { ...prev, validating: false }; return res.ok ? { ...prev, validating: false, validated: true } : { ...prev, validating: false, validated: false, error: res.message }; });
-
[Major] Newly added IPC contracts are still unversioned, which violates the compatibility requirement for IPC surfaces (
apps/desktop/src/main/onboarding-ipc.ts:202,apps/desktop/src/main/preferences-ipc.ts:86).
Suggested fix:ipcMain.handle('settings:v1:list-providers', () => { return toProviderRows(getCachedConfig(), decryptSecret); }); ipcMain.handle('preferences:v1:get', async () => readPersisted());
-
[Major] New UI markup introduces hardcoded visual literals (
text-white,text-[10px]) instead of tokenized values, violating the token-only UI rule (apps/desktop/src/renderer/src/components/Settings.tsx:435,apps/desktop/src/renderer/src/components/Settings.tsx:440).
Suggested fix:<span className="inline-flex items-center px-1.5 py-0.5 rounded-full bg-[var(--color-accent)] text-[var(--color-text-inverse)] text-[var(--text-2xs)] font-medium leading-none"> Active </span>
Summary
- Review mode: follow-up after new commits
- 4 issues remain on the current head: active-provider delete keeps stale models, async validation race, unversioned new IPC channels, and token-rule violations in newly added UI classes.
Testing
- Not run (automation).
- Missing coverage for deleting the active provider with provider switch, and for stale validation responses while form fields change mid-request.
open-codesign Bot
| const next: Config = { | ||
| version: 1, | ||
| provider: nextActive, | ||
| modelPrimary: cfg.modelPrimary, |
There was a problem hiding this comment.
Switching active provider here keeps cfg.modelPrimary/cfg.modelFast from the deleted provider, which can persist an invalid provider/model pair.
Suggested fix:
const switchedActive = cfg.provider === raw;
const defaults = switchedActive ? PROVIDER_SHORTLIST[nextActive] : null;
const next: Config = {
version: 1,
provider: nextActive,
modelPrimary: defaults?.defaultPrimary ?? cfg.modelPrimary,
modelFast: defaults?.defaultFast ?? cfg.modelFast,
secrets: nextSecrets,
baseUrls: nextBaseUrls,
};| ...(trimmedUrl.length > 0 ? { baseUrl: trimmedUrl } : {}), | ||
| }); | ||
| if (res.ok) { | ||
| setForm((prev) => ({ ...prev, validating: false, validated: true })); |
There was a problem hiding this comment.
This async completion can mark edited credentials as validated if provider/key/base URL changes while validation is in flight.
Suggested fix:
const snapshot = {
provider: form.provider,
apiKey: form.apiKey.trim(),
baseUrl: form.baseUrl.trim(),
};
const res = await window.codesign.settings.validateKey({
provider: snapshot.provider,
apiKey: snapshot.apiKey,
...(snapshot.baseUrl ? { baseUrl: snapshot.baseUrl } : {}),
});
setForm((prev) => {
const stale =
prev.provider !== snapshot.provider ||
prev.apiKey.trim() !== snapshot.apiKey ||
prev.baseUrl.trim() !== snapshot.baseUrl;
if (stale) return { ...prev, validating: false };
return res.ok
? { ...prev, validating: false, validated: true }
: { ...prev, validating: false, validated: false, error: res.message };
});|
|
||
| // ── Settings: provider management ────────────────────────────────────────── | ||
|
|
||
| ipcMain.handle('settings:list-providers', (): ProviderRow[] => { |
There was a problem hiding this comment.
New settings IPC channels are unversioned. The project compatibility rule requires versioned IPC contracts to avoid future breaking changes.
Suggested fix:
ipcMain.handle(settings:v1:list-providers, () => {
return toProviderRows(getCachedConfig(), decryptSecret);
});| } | ||
|
|
||
| export function registerPreferencesIpc(): void { | ||
| ipcMain.handle('preferences:get', async (): Promise<Preferences> => { |
There was a problem hiding this comment.
These newly added preferences IPC channels are also unversioned; they should follow the same :v1: convention as other public contracts.
Suggested fix:
ipcMain.handle(preferences:v1:get, async (): Promise<Preferences> => {
return readPersisted();
});| {label} | ||
| </span> | ||
| {row.isActive && !hasError && ( | ||
| <span className="inline-flex items-center px-1.5 py-0.5 rounded-full bg-[var(--color-accent)] text-white text-[10px] font-medium leading-none"> |
There was a problem hiding this comment.
This new class uses hardcoded visual literals (text-white, text-[10px]), which violates the token-only UI rule.
Suggested fix:
<span className="inline-flex items-center px-1.5 py-0.5 rounded-full bg-[var(--color-accent)] text-[var(--color-text-inverse)] text-[var(--text-2xs)] font-medium leading-none">
Active
</span>…te against stale form - Extract computeDeleteProviderResult pure helper to provider-settings.ts; when the active provider is deleted the new active provider's PROVIDER_SHORTLIST defaults are used for modelPrimary/modelFast instead of carrying over the old provider's model IDs (Blocker from #16 review). - Tombstone path (last provider removed) now writes empty model strings so a stale model ID is never persisted. - Guard handleValidate in AddProviderModal: snapshot provider/apiKey/baseUrl before the async call and discard the result via applyValidateResult if the form changed while awaiting (Major race fix from #16 review). - Add applyValidateResult pure exported helper for unit testing without a DOM. - 5 new tests (3 computeDeleteProviderResult + 4 applyValidateResult, total 15). Signed-off-by: hqhq1025 <1506751656@qq.com>
There was a problem hiding this comment.
Findings
-
[Major] New settings IPC contracts are unversioned, which violates the compatibility requirement for IPC surfaces and makes future contract evolution breaking by default. Evidence
apps/desktop/src/main/onboarding-ipc.ts:203,apps/desktop/src/preload/index.ts:93
Suggested fix:// main ipcMain.handle('settings:v1:list-providers', () => toProviderRows(getCachedConfig(), decryptSecret)); // preload listProviders: () => ipcRenderer.invoke('settings:v1:list-providers') as Promise<ProviderRow[]>,
-
[Major] New preferences IPC contracts are also unversioned; this introduces another public contract without versioning and conflicts with the same compatibility rule. Evidence
apps/desktop/src/main/preferences-ipc.ts:86,apps/desktop/src/preload/index.ts:122
Suggested fix:// main ipcMain.handle('preferences:v1:get', async () => readPersisted()); ipcMain.handle('preferences:v1:update', async (_e, raw: unknown) => { const patch = parsePreferences(raw); const current = await readPersisted(); const next: Preferences = { ...current, ...patch }; await writePersisted(next); return next; }); // preload get: () => ipcRenderer.invoke('preferences:v1:get') as Promise<Preferences>, update: (patch: Partial<Preferences>) => ipcRenderer.invoke('preferences:v1:update', patch) as Promise<Preferences>,
-
[Major] New Settings badges/buttons use hardcoded visual literals (
text-white,text-[10px]) instead of UI tokens, violating the token-only UI rule and causing style drift risk. Evidenceapps/desktop/src/renderer/src/components/Settings.tsx:474,apps/desktop/src/renderer/src/components/Settings.tsx:479,apps/desktop/src/renderer/src/components/Settings.tsx:488
Suggested fix:<span className="inline-flex items-center px-1.5 py-0.5 rounded-full bg-[var(--color-accent)] text-[var(--color-text-inverse)] text-[var(--text-2xs)] font-medium leading-none"> Active </span>
Summary
- Review mode: follow-up after new commits
- 3 issues remain on the current head: two newly introduced unversioned IPC surfaces (settings + preferences) and token-rule violations in new Settings UI classes.
Testing
- Not run (automation).
- Missing coverage/guardrails for IPC version naming conventions and token-only class usage in newly added UI markup.
open-codesign Bot
|
|
||
| // ── Settings: provider management ────────────────────────────────────────── | ||
|
|
||
| ipcMain.handle('settings:list-providers', (): ProviderRow[] => { |
There was a problem hiding this comment.
New settings IPC channels are introduced unversioned (settings:list-providers, etc.). This breaks the compatibility/versioning rule for IPC contracts.
Suggested fix:
ipcMain.handle("settings:v1:list-providers", () => toProviderRows(getCachedConfig(), decryptSecret));| } | ||
|
|
||
| export function registerPreferencesIpc(): void { | ||
| ipcMain.handle('preferences:get', async (): Promise<Preferences> => { |
There was a problem hiding this comment.
preferences:get/preferences:update are newly introduced public IPC contracts without versioning.
Suggested fix:
ipcMain.handle("preferences:v1:get", async () => readPersisted());
ipcMain.handle("preferences:v1:update", async (_e, raw: unknown) => { /* ... */ });| {label} | ||
| </span> | ||
| {row.isActive && !hasError && ( | ||
| <span className="inline-flex items-center px-1.5 py-0.5 rounded-full bg-[var(--color-accent)] text-white text-[10px] font-medium leading-none"> |
There was a problem hiding this comment.
This new UI class uses hardcoded literals (text-white, text-[10px]), which violates the token-only UI rule.
Suggested fix:
<span className="... text-[var(--color-text-inverse)] text-[var(--text-2xs)] ...">Active</span>…te against stale form - Extract computeDeleteProviderResult pure helper to provider-settings.ts; when the active provider is deleted the new active provider's PROVIDER_SHORTLIST defaults are used for modelPrimary/modelFast instead of carrying over the old provider's model IDs (Blocker from #16 review). - Tombstone path (last provider removed) now writes empty model strings so a stale model ID is never persisted. - Guard handleValidate in AddProviderModal: snapshot provider/apiKey/baseUrl before the async call and discard the result via applyValidateResult if the form changed while awaiting (Major race fix from #16 review). - Add applyValidateResult pure exported helper for unit testing without a DOM. - 5 new tests (3 computeDeleteProviderResult + 4 applyValidateResult, total 15). Signed-off-by: hqhq1025 <1506751656@qq.com>
hqhq1025
force-pushed
the
wt/settings-real
branch
from
50c21b4 to
5ae1d27
Compare
…rences persistence Implements all four settings tabs (Models, Appearance, Storage, Advanced) with full persistence and correct error handling. Key changes: - Settings UI: four-tab panel (Models / Appearance / Storage / Advanced) with provider cards, add/delete/activate flows, model selector, path viewer, reset - toProviderRows: soft-fail on safeStorage decryption — returns error:'decryption_failed' row instead of throwing; UI shows red badge + 'Re-enter key' button - preferences-ipc.ts: new IPC module persisting updateChannel and generationTimeoutSec to ~/.config/open-codesign/preferences.json (schemaVersion:1) - locale-ipc.ts and preferences-ipc.ts registered in main/index.ts - preload: settings, preferences, and locale namespaces exposed to renderer - AppearanceTab: language select reads/writes via locale:get-current / locale:set - AdvancedTab: update channel and generation timeout read/write via preferences IPC - ActiveModelSelector: 400ms debounce with proper useEffect cleanup on unmount - delete-provider: fix ghost active provider — when all providers removed, write tombstone config (empty secrets) instead of falling back to hardcoded 'openai'; re-adding a new provider correctly auto-activates it - provider-settings.ts and onboarding-ipc.ts: use electron-runtime import pattern - Tests: 8 total (5 new), covering decryption-failed row, masked-key row, ghost-provider auto-activate, provider preservation, assertProviderHasStoredSecret Signed-off-by: hqhq1025 <1506751656@qq.com>
…te against stale form - Extract computeDeleteProviderResult pure helper to provider-settings.ts; when the active provider is deleted the new active provider's PROVIDER_SHORTLIST defaults are used for modelPrimary/modelFast instead of carrying over the old provider's model IDs (Blocker from #16 review). - Tombstone path (last provider removed) now writes empty model strings so a stale model ID is never persisted. - Guard handleValidate in AddProviderModal: snapshot provider/apiKey/baseUrl before the async call and discard the result via applyValidateResult if the form changed while awaiting (Major race fix from #16 review). - Add applyValidateResult pure exported helper for unit testing without a DOM. - 5 new tests (3 computeDeleteProviderResult + 4 applyValidateResult, total 15). Signed-off-by: hqhq1025 <1506751656@qq.com>
- Add settings:v1:* and preferences:v1:* versioned IPC channels (8 settings + 2 prefs) - Keep legacy settings:* and preferences:* channels as shims with logger.warn - Preload now calls v1 channels exclusively - Extract per-handler run* helpers shared by v1 and legacy registrations - Replace text-white with text-[var(--color-on-accent)] in Settings badges/buttons - Replace text-[10px] with text-[var(--font-size-badge)] in ProviderCard badges - Add --color-on-accent and --font-size-badge tokens to packages/ui/src/tokens.css - Add onboarding-ipc.test.ts: 3 tests covering v1 channel registration and legacy compat Signed-off-by: hqhq1025 <1506751656@qq.com>
- Settings.tsx locale-load .catch(() => {}) → pushToast error
- Settings.tsx preferences bootstrap .catch(() => {}) → pushToast error
- preferences-ipc.ts non-ENOENT read errors → throw CodesignError (PREFERENCES_READ_FAILED)
- packages/core generate() empty artifacts → throw CodesignError (OUTPUT_MISSING_ARTIFACT)
- Add pushIframeError to CodesignState interface (was implemented but missing from type)
- Fix store.test.ts: remove stale designSystem field not present in OnboardingState
- Add preferences-ipc.test.ts: 2 tests covering ENOENT default and non-ENOENT throw
- Add generate.test.ts: 4 tests covering OUTPUT_MISSING_ARTIFACT error path
Signed-off-by: hqhq1025 <1506751656@qq.com>
There was a problem hiding this comment.
Findings
- No actionable issues found on added/modified lines in the current head.
Summary
- Review mode: follow-up after new commits
- No new issues found in this pass.
- Residual risk: the new settings/provider flows are covered by Vitest, but end-to-end behavior for Settings tabs (Models/Storage/Advanced) and IPC wiring is still not covered by Playwright in this PR.
Testing
- Not run (automation)
- Suggested: add one Playwright smoke test that opens Settings and exercises provider add/switch/delete + preferences read/write roundtrip.
open-codesign Bot
Add @open-codesign/i18n workspace dep, LocalInputFile/ElementSelection types from main, and fix sendPrompt call signature in App.tsx to match the object-input type declared in store.ts.
The PR commits were originally based on an older version of the codebase and accidentally removed features added by PRs #18 and #20: - Restore design-system.ts and prompt-context.ts (deleted in branch) - Restore InlineCommentComposer.tsx and PreviewPane.test.ts (deleted) - Restore StoredDesignSystem / designSystem support in config.ts - Re-export StoredDesignSystem + STORED_DESIGN_SYSTEM_SCHEMA_VERSION from shared - Re-add getOnboardingState() / setDesignSystem() to onboarding-ipc.ts (these are needed by main/index.ts design-system IPC handlers) - Add full import set to main/index.ts (stat, basename, applyComment, dialog, scanDesignSystem, getCachedConfig, getOnboardingState, setDesignSystem, preparePromptContext) - Restore preload/index.ts: add back applyComment, pickInputFiles, pickDesignSystemDirectory, clearDesignSystem methods removed in PR; keep PR-added settings/preferences namespaces and interfaces - Restore all other files regressed to pre-#18/#20 state (Sidebar.tsx, store.ts, App.tsx, core/index.ts, providers, templates, ui tokens, etc.) - Fix Settings.tsx formatting (spurious blank line) - Fix store.test.ts READY_CONFIG missing designSystem field
hqhq1025
force-pushed
the
wt/settings-real
branch
from
5ae1d27 to
838885d
Compare
2 participants
Summary
New IPC channels (all in
onboarding-ipc.ts)settings:list-providersProviderRow[]with masked keyssettings:add-providersettings:delete-providersettings:set-active-providersettings:get-pathssettings:open-foldersettings:reset-onboardingsettings:toggle-devtoolswebContents.toggleDevTools()New config.toml fields
No schema changes — the existing
provider,modelPrimary,modelFast,secrets, andbaseUrlsfields carry all the new state.provideralready acts asactiveProvider.Design principles check
anyTest plan
pnpm --filter @open-codesign/desktop typecheck— no errorspnpm lint— no errors (1 pre-existing warning in PasteKey.tsx unrelated)