fix(dashboard): repair Playground model selector for custom providers (#3731) - #3772
Conversation
…#3731) Two bugs made the Playground model selector unusable for custom OpenAI/Anthropic-compatible providers: - when the catalog prefix didn't resolve, the list was filtered by the raw connection id (matches nothing) → empty selector ('NONE shown'); - selecting a provider reset the model to '' and nothing picked a default → chat failed with 'Set a model in the config pane'. Extract two pure helpers (resolveModelFilterKey, pickDefaultModel): a compatible provider without a prefix now falls back to the full catalog instead of emptying, and the first available model is auto-selected once the list resolves. Closes #3731
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
There was a problem hiding this comment.
Code Review
This pull request fixes bugs in the Playground model selector for custom OpenAI/Anthropic-compatible providers by introducing helper functions to resolve model filter keys and auto-select default models, along with unit tests. The review feedback focuses on improving robustness through defensive checks for undefined or non-string inputs, refactoring the state update in the useEffect hook to use a functional updater to comply with ESLint rules, and adding corresponding test coverage.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| const isCompatibleConnectionId = | ||
| provider.startsWith(OPENAI_COMPATIBLE_PREFIX) || | ||
| provider.startsWith(ANTHROPIC_COMPATIBLE_PREFIX) || | ||
| provider.startsWith(CLAUDE_CODE_COMPATIBLE_PREFIX); |
There was a problem hiding this comment.
To prevent potential runtime crashes (e.g., TypeError: Cannot read properties of undefined (reading 'startsWith')), we should add a defensive check to ensure provider is a valid string before calling .startsWith.
const isCompatibleConnectionId =
typeof provider === "string" && (
provider.startsWith(OPENAI_COMPATIBLE_PREFIX) ||
provider.startsWith(ANTHROPIC_COMPATIBLE_PREFIX) ||
provider.startsWith(CLAUDE_CODE_COMPATIBLE_PREFIX)
);
| useEffect(() => { | ||
| const next = pickDefaultModel(configState.model, availableModels); | ||
| if (next !== null) setConfigState({ ...configState, model: next }); | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| }, [availableModels, configState.model]); |
There was a problem hiding this comment.
To avoid disabling the ESLint react-hooks/exhaustive-deps rule and ensure full compliance with React best practices, we can use a functional state update for setConfigState. This removes the dependency on the full configState object and prevents potential stale closure issues.
| useEffect(() => { | |
| const next = pickDefaultModel(configState.model, availableModels); | |
| if (next !== null) setConfigState({ ...configState, model: next }); | |
| // eslint-disable-next-line react-hooks/exhaustive-deps | |
| }, [availableModels, configState.model]); | |
| useEffect(() => { | |
| const next = pickDefaultModel(configState.model, availableModels); | |
| if (next !== null) { | |
| setConfigState((prev) => ({ ...prev, model: next })); | |
| } | |
| }, [availableModels, configState.model, setConfigState]); |
| export function resolveModelFilterKey( | ||
| provider: string, | ||
| modelPrefix: string | undefined, | ||
| isCompatibleConnectionId: boolean | ||
| ): string | undefined { |
There was a problem hiding this comment.
To make resolveModelFilterKey more robust and prevent potential runtime errors, we should allow provider to be string | undefined and handle it gracefully.
| export function resolveModelFilterKey( | |
| provider: string, | |
| modelPrefix: string | undefined, | |
| isCompatibleConnectionId: boolean | |
| ): string | undefined { | |
| export function resolveModelFilterKey( | |
| provider: string | undefined, | |
| modelPrefix: string | undefined, | |
| isCompatibleConnectionId: boolean | |
| ): string | undefined { |
| export function pickDefaultModel( | ||
| currentModel: string | undefined, | ||
| availableModels: string[] | ||
| ): string | null { | ||
| if (availableModels.length === 0) return null; |
There was a problem hiding this comment.
To prevent potential runtime crashes if availableModels is undefined (e.g., during initial load or if the hook fails), we should add a defensive check to handle undefined or null values for availableModels.
| export function pickDefaultModel( | |
| currentModel: string | undefined, | |
| availableModels: string[] | |
| ): string | null { | |
| if (availableModels.length === 0) return null; | |
| export function pickDefaultModel( | |
| currentModel: string | undefined, | |
| availableModels: string[] | undefined | |
| ): string | null { | |
| if (!availableModels || availableModels.length === 0) return null; |
|
|
||
| test("pickDefaultModel: empty list selects nothing", () => { | ||
| assert.equal(pickDefaultModel("", []), null); | ||
| assert.equal(pickDefaultModel("gpt-4o", []), null); |
There was a problem hiding this comment.
Since we updated pickDefaultModel to handle undefined lists, we should add a test case to verify this behavior.
| test("pickDefaultModel: empty list selects nothing", () => { | |
| assert.equal(pickDefaultModel("", []), null); | |
| assert.equal(pickDefaultModel("gpt-4o", []), null); | |
| test("pickDefaultModel: empty or undefined list selects nothing", () => { | |
| assert.equal(pickDefaultModel("", []), null); | |
| assert.equal(pickDefaultModel("gpt-4o", []), null); | |
| assert.equal(pickDefaultModel("", undefined), null); | |
| assert.equal(pickDefaultModel("gpt-4o", undefined), null); | |
| }); |
…diegosouzapw#3731) (diegosouzapw#3772) Two bugs made the Playground model selector unusable for custom OpenAI/Anthropic-compatible providers: - when the catalog prefix didn't resolve, the list was filtered by the raw connection id (matches nothing) → empty selector ('NONE shown'); - selecting a provider reset the model to '' and nothing picked a default → chat failed with 'Set a model in the config pane'. Extract two pure helpers (resolveModelFilterKey, pickDefaultModel): a compatible provider without a prefix now falls back to the full catalog instead of emptying, and the first available model is auto-selected once the list resolves. Closes diegosouzapw#3731
Closes #3731 (dup #3009)
Problem
In the dashboard Playground, a custom OpenAI/Anthropic-compatible provider's model selector showed NONE — you couldn't select a model even after fetching/importing them. Two distinct defects:
StudioConfigPanefiltered the model list byselectedProviderOption?.modelPrefix || provider. When the option's catalog prefix didn't resolve, it fell back to the raw connection id (e.g.openai-compatible-<uuid>), which matches nothing in the catalog → empty list → the selector degraded to the free-text input.configState.modelto"", and nothing ever picked a default. Even when the dropdown had options, the active model stayed empty, so the chat errored with "Set a model in the config pane".Fix
Two pure, unit-tested helpers in
modelSelection.ts, wired intoStudioConfigPane:resolveModelFilterKey()— a compatible/custom connection without a resolved prefix now returnsundefined(show the full catalog) instead of filtering by the connection id, so the selector is never silently empty.pickDefaultModel()— auseEffectauto-selects the first available model when the list resolves and the current model is empty/invalid (mirrors the working provider-detail chat).Validation (Hard Rule #18 — TDD)
tests/unit/playground-model-selection-3731.test.ts— 8 tests covering both helpers (built-in vs compatible filter keys, the no-prefix→full-catalog fallback, and auto-select / keep-valid / empty-list). Apptscclean on the changed.tsx, eslint + prettier clean.