diff --git a/apps/desktop/src/components/settings/ImageGenerationModelRow.tsx b/apps/desktop/src/components/settings/ImageGenerationModelRow.tsx index 070117fee..58486f921 100644 --- a/apps/desktop/src/components/settings/ImageGenerationModelRow.tsx +++ b/apps/desktop/src/components/settings/ImageGenerationModelRow.tsx @@ -1,14 +1,15 @@ import { useTranslation } from "react-i18next"; import { - imageGenerationBindings, - vendorAccountImageCandidates, type AppSettings, type ImageGenerationBinding, type ProviderPublic, } from "@pi-desktop/shared"; import { sameComposerModelId } from "../../lib/composer-models"; +import { + imageGenerationBindingAvailable, + imageGenerationPickerCandidates, +} from "./image-generation-default"; import { SettingsMenuSelect } from "./SettingsMenuSelect"; -import { imageGenerationBindingAvailable } from "./image-generation-default"; function imageModelOptionId(binding: ImageGenerationBinding): string { return `${binding.providerId}\u0000${binding.modelId}`; @@ -27,16 +28,15 @@ export function ImageGenerationModelRow({ }) { const { t } = useTranslation(); const binding = settings.imageGeneration; - // An explicit candidate list is the user's selection. Do not put the stored - // default back when they cleared it; only a missing list is the legacy - // single-binding fallback. A signed-in vendor account also offers the image - // model it answers with, which its chat model list does not carry. - const configured = Array.isArray(settings.imageGenerationModels) - ? imageGenerationBindings(settings.imageGenerationModels, null) - : imageGenerationBindings(undefined, binding); - const candidates = imageGenerationBindings( - [...configured, ...vendorAccountImageCandidates(providers)], - null, + // The picker's options: the candidate list stored in settings is the user's + // selection (an explicitly cleared list is not resurrected), a missing list + // falls back to the legacy single binding, and a signed-in vendor account + // offers the image model it answers with. The settings page validates the + // user's choice against this same list, so every row shown here is selectable. + const candidates = imageGenerationPickerCandidates( + settings.imageGenerationModels, + binding, + providers, ); if (candidates.length === 0) return null; diff --git a/apps/desktop/src/components/settings/ModelConfigPage.tsx b/apps/desktop/src/components/settings/ModelConfigPage.tsx index 3416fafab..0b6cc025e 100644 --- a/apps/desktop/src/components/settings/ModelConfigPage.tsx +++ b/apps/desktop/src/components/settings/ModelConfigPage.tsx @@ -24,7 +24,11 @@ import { IconServer, } from "../icons"; import { providerServesChatModels } from "./default-model"; -import { planImageGenerationDefaults } from "./image-generation-default"; +import { + imageGenerationPickerCandidates, + isImageGenerationPickerCandidate, + planImageGenerationDefaults, +} from "./image-generation-default"; import { copyProviderConfiguration, type ProviderCopyDraft } from "./provider-copy"; import { ImageGenerationModelRow } from "./ImageGenerationModelRow"; import { OAuthLoginDialog } from "./OAuthLoginDialog"; @@ -67,16 +71,12 @@ function imageCandidates( return result; } -function isImageCandidate(candidates: readonly ImageGenerationBinding[], providerId: string, modelId: string) { - return candidates.some((entry) => entry.providerId === providerId && sameWireId(entry.modelId, modelId)); -} - function chatModelOptions(providers: readonly ProviderPublic[], imageModels: readonly ImageGenerationBinding[]) { return providers.flatMap((provider) => { const ids = provider.models?.length ? provider.models.map((model) => model.id) : [provider.defaultModelId ?? ""]; - return ids.filter((id) => !!id.trim() && !isImageCandidate(imageModels, provider.id, id)) + return ids.filter((id) => !!id.trim() && !isImageGenerationPickerCandidate(imageModels, provider.id, id)) .map((modelId) => ({ provider, modelId })); }); } @@ -226,11 +226,17 @@ export function ModelConfigPage() { setChangingImageModel(true); try { const current = await api.getSettings(); - const candidates = imageCandidates( + // Exactly the list the picker row offered, so a choice the user could + // make is always one this page accepts — including the image model of a + // signed-in vendor account, which is never stored as a chat model. + const candidates = imageGenerationPickerCandidates( current.imageGenerationModels, current.imageGeneration, + providers, ); - if (!isImageCandidate(candidates, binding.providerId, binding.modelId)) return; + if (!isImageGenerationPickerCandidate(candidates, binding.providerId, binding.modelId)) { + return; + } const nextSettings = { ...current, imageGeneration: binding }; await api.setSettings(nextSettings); useAppStore.setState({ settings: nextSettings }); diff --git a/apps/desktop/src/components/settings/image-generation-default.ts b/apps/desktop/src/components/settings/image-generation-default.ts index 82a11bd60..b196633e6 100644 --- a/apps/desktop/src/components/settings/image-generation-default.ts +++ b/apps/desktop/src/components/settings/image-generation-default.ts @@ -22,7 +22,9 @@ import { MAX_IMAGE_GENERATION_MODELS, imageGenerationBindings, imageModelOfferedByProvider, + vendorAccountImageCandidates, type ImageGenerationBinding, + type ImageProviderFacts, type ProviderPublic, modelWireIdsEqual as sameComposerModelId, } from "@pi-desktop/shared"; @@ -58,6 +60,43 @@ export function resolvesImageGenerationDefault( return imageGenerationBindingAvailable(provider, binding.modelId); } +/** + * Every binding the image-model picker offers, in the order it shows them: a + * stored candidate list is the user's own selection, the single stored default + * is the legacy fallback only while no list was ever saved, and a signed-in + * vendor account contributes the image model it answers with — which its chat + * model list does not carry. + * + * The row that renders these options and the settings page that accepts the + * user's choice must read this one list. When they composed it separately, the + * page validated against the narrower one, so picking the image model of a + * signed-in ChatGPT (Codex) account saved nothing at all. + */ +export function imageGenerationPickerCandidates( + stored: readonly ImageGenerationBinding[] | null | undefined, + active: ImageGenerationBinding | null | undefined, + providers: readonly ImageProviderFacts[], +): ImageGenerationBinding[] { + const configured = Array.isArray(stored) + ? imageGenerationBindings(stored, null) + : imageGenerationBindings(undefined, active); + return imageGenerationBindings( + [...configured, ...vendorAccountImageCandidates(providers)], + null, + ); +} + +/** Whether `(providerId, modelId)` is one of the candidates a picker offered. */ +export function isImageGenerationPickerCandidate( + candidates: readonly ImageGenerationBinding[], + providerId: string, + modelId: string, +): boolean { + return candidates.some((entry) => + entry.providerId === providerId && sameComposerModelId(entry.modelId, modelId), + ); +} + export type ImageGenerationDefaultDraft = { imageGenerationModels?: readonly ImageGenerationBinding[] | null; imageGeneration?: ImageGenerationBinding | null; diff --git a/apps/desktop/test/image-generation-default.test.mjs b/apps/desktop/test/image-generation-default.test.mjs index ed67add1f..b395253ff 100644 --- a/apps/desktop/test/image-generation-default.test.mjs +++ b/apps/desktop/test/image-generation-default.test.mjs @@ -15,6 +15,7 @@ * must not accept it as proof that a default can run. */ import assert from "node:assert/strict"; +import { readFile } from "node:fs/promises"; import { register } from "node:module"; import test from "node:test"; @@ -22,6 +23,8 @@ import test from "node:test"; register(new URL("./helpers/ts-import-hooks.mjs", import.meta.url)); const { imageGenerationBindingAvailable, + imageGenerationPickerCandidates, + isImageGenerationPickerCandidate, planImageGenerationDefaults, resolvesImageGenerationDefault, } = await import("../src/components/settings/image-generation-default.ts"); @@ -416,3 +419,58 @@ test("the Codex image model is offered as a candidate without being stored as a ); assert.deepEqual(plan.imageGeneration, binding("codex", "gpt-image-2")); }); + +/** + * The picker's own list is the only list a selection may be validated against. + * The row drew from the stored candidates plus a signed-in vendor account's + * image model, while the settings page checked the stored candidates alone, so + * choosing the ChatGPT (Codex) model did nothing at all. + */ +test("the picker offers every candidate it accepts, vendor accounts included", () => { + const candidates = imageGenerationPickerCandidates( + undefined, + null, + [codexAccount(), provider("x", [])], + ); + assert.deepEqual(candidates, [ + binding("codex", "gpt-image-2.5"), + binding("codex", "gpt-image-2"), + ]); + assert.equal(isImageGenerationPickerCandidate(candidates, "codex", "gpt-image-2.5"), true); + assert.equal(isImageGenerationPickerCandidate(candidates, "codex", "GPT-IMAGE-2.5"), true); + // Nothing the row never offered may be accepted: another provider, another + // model, or the account's chat model. + assert.equal(isImageGenerationPickerCandidate(candidates, "x", "gpt-image-2.5"), false); + assert.equal(isImageGenerationPickerCandidate(candidates, "codex", "gpt-image-3"), false); + assert.equal(isImageGenerationPickerCandidate(candidates, "codex", "gpt-6.1-sol"), false); + // A signed-out account, or another vendor's login, offers no image model. + assert.deepEqual( + imageGenerationPickerCandidates(undefined, null, [ + codexAccount({ hasOauth: false }), + provider("a", [], { vendorKey: "anthropic", authKind: "oauth", hasOauth: true }), + ]), + [], + ); + // A stored candidate list outranks the legacy single-binding fallback in + // both directions: it is offered, while the cleared default is not restored. + assert.deepEqual( + imageGenerationPickerCandidates([binding("x", "img-x")], binding("y", "img-y"), []), + [binding("x", "img-x")], + ); + assert.deepEqual( + imageGenerationPickerCandidates(undefined, binding("y", "img-y"), []), + [binding("y", "img-y")], + ); +}); + +test("the settings row and the settings page compose one candidate list", async () => { + const read = (rel) => readFile(new URL(rel, import.meta.url), "utf8"); + const [row, page] = await Promise.all([ + read("../src/components/settings/ImageGenerationModelRow.tsx"), + read("../src/components/settings/ModelConfigPage.tsx"), + ]); + // Rendering the options and validating the choice must read the same helper. + assert.match(row, /imageGenerationPickerCandidates\(/); + assert.match(page, /imageGenerationPickerCandidates\(/); + assert.match(page, /isImageGenerationPickerCandidate\(/); +}); diff --git a/apps/desktop/test/model-advanced-capabilities.test.mjs b/apps/desktop/test/model-advanced-capabilities.test.mjs index 8c8fb94e4..8e1010564 100644 --- a/apps/desktop/test/model-advanced-capabilities.test.mjs +++ b/apps/desktop/test/model-advanced-capabilities.test.mjs @@ -97,7 +97,10 @@ test("image generation selection hides the summary when nothing can be chosen", ); assert.match(pickerSource, /imageModelIds\?\.some\([\s\S]*?modelId\.toLowerCase\(\) === binding\.id\.toLowerCase\(\)/); assert.match(pickerSource, /onImageModelChange\(binding\.id, event\.target\.checked\)/); - assert.match(imageModelRowSource, /imageGenerationBindings\(settings\.imageGenerationModels, null\)/); + // The row's own rule — the stored candidate list, the legacy single binding + // only while no list was ever saved, plus a signed-in vendor account's image + // model — now lives in the shared helper, pinned by image-generation-default.test.mjs. + assert.ok(imageModelRowSource.includes("imageGenerationPickerCandidates(")); assert.match(imageModelRowSource, /if \(!options\.some\(\(option\) => !option\.disabled\)\) return null;/); assert.match(imageModelRowSource, /if \(candidates\.length === 0\) return null;/); assert.match(imageModelRowSource, /imageModelUnavailable/); diff --git a/apps/desktop/test/provider-model-config.test.mjs b/apps/desktop/test/provider-model-config.test.mjs index 8e27589d7..e0f58949b 100644 --- a/apps/desktop/test/provider-model-config.test.mjs +++ b/apps/desktop/test/provider-model-config.test.mjs @@ -359,7 +359,9 @@ test("settings match complete case-normalized wire ids, not proxy suffixes", () const sameWireId = new Function("left", "right", `return ${identity[1]}`); assert.equal(sameWireId("PROXY/model", "proxy/MODEL"), true); assert.equal(sameWireId("proxy/model", "model"), false); - assert.match(pageSource, /isImageCandidate\(imageModels, provider\.id, id\)/); + // Image candidates are excluded by the same complete-id membership rule the + // picker row and the page's own selection check share. + assert.ok(pageSource.includes("isImageGenerationPickerCandidate(imageModels, provider.id, id)")); assert.match(pageSource, /!models\.some\(\(model\) => sameWireId\(model\.id, settings\.defaultModelId/); assert.doesNotMatch(pageSource, /modelIdsMatch|isImageGenerationModel/); assert.doesNotMatch(setupSource, /modelIdsMatch/);