Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 13 additions & 13 deletions apps/desktop/src/components/settings/ImageGenerationModelRow.tsx
Original file line number Diff line number Diff line change
@@ -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}`;
Expand All @@ -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;

Expand Down
22 changes: 14 additions & 8 deletions apps/desktop/src/components/settings/ModelConfigPage.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand Down Expand Up @@ -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 }));
});
}
Expand Down Expand Up @@ -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 });
Expand Down
39 changes: 39 additions & 0 deletions apps/desktop/src/components/settings/image-generation-default.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand Down Expand Up @@ -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;
Expand Down
58 changes: 58 additions & 0 deletions apps/desktop/test/image-generation-default.test.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -15,13 +15,16 @@
* 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";

// The renderer module resolves its sibling through the bundler, not Node.
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");
Expand Down Expand Up @@ -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\(/);
});
5 changes: 4 additions & 1 deletion apps/desktop/test/model-advanced-capabilities.test.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -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/);
Expand Down
4 changes: 3 additions & 1 deletion apps/desktop/test/provider-model-config.test.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -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/);
Expand Down
Loading