Skip to content

Commit 5912186

Browse files
ibetitsmikeibetitsmike
committed
fix(coderd,site): address PR #23647 review feedback
1 parent eb93ee4 commit 5912186

File tree

4 files changed

+58
-23
lines changed

4 files changed

+58
-23
lines changed

‎coderd/database/queries.sql.go‎

Lines changed: 3 additions & 1 deletion
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎coderd/database/queries/chatmodelconfigs.sql‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -60,11 +60,13 @@ WHERE
6060
AND cp.enabled = TRUE
6161
))
6262
OR
63-
-- Unbound with no provider rows at all (env-only family).
63+
-- Unbound with no enabled provider rows (env-only family or
64+
-- all provider configs disabled).
6465
(cmc.provider_config_id IS NULL
6566
AND NOT EXISTS (
6667
SELECT 1 FROM chat_providers cp
6768
WHERE cp.provider = cmc.provider
69+
AND cp.enabled = TRUE
6870
))
6971
)
7072
ORDER BY

‎coderd/exp_chats.go‎

Lines changed: 12 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -622,6 +622,7 @@ func (api *API) listChatModels(rw http.ResponseWriter, r *http.Request) {
622622
}
623623
enabledProviderNames[provider] = struct{}{}
624624
}
625+
visibleProviderNames := make(map[string]struct{}, len(visibleProviders))
625626
for _, provider := range visibleProviders {
626627
configuredProviders = append(
627628
configuredProviders, chatprovider.ConfiguredProvider{
@@ -638,8 +639,17 @@ func (api *API) listChatModels(rw http.ResponseWriter, r *http.Request) {
638639
if normalizedProvider == "" {
639640
continue
640641
}
642+
visibleProviderNames[normalizedProvider] = struct{}{}
641643
enabledProviderNames[normalizedProvider] = struct{}{}
642644
}
645+
if !isAdmin {
646+
for provider := range enabledProviderNames {
647+
if _, ok := visibleProviderNames[provider]; ok {
648+
continue
649+
}
650+
delete(enabledProviderNames, provider)
651+
}
652+
}
643653
configuredModels := make(
644654
[]chatprovider.ConfiguredModel, 0, len(visibleModels),
645655
)
@@ -5461,31 +5471,27 @@ func shouldCleanUnboundModelsAfterProviderDelete(
54615471
remainingEnabledProviders := make(
54625472
[]database.ChatProvider, 0, len(enabledProviders),
54635473
)
5464-
remainingEnabledInFamily := 0
54655474
for _, provider := range enabledProviders {
54665475
if provider.ID == deletedProvider.ID {
54675476
continue
54685477
}
54695478
remainingEnabledProviders = append(remainingEnabledProviders, provider)
5470-
if provider.Provider == deletedProvider.Provider {
5471-
remainingEnabledInFamily++
5472-
}
54735479
}
54745480
if defaultChatProviderAPIKeys(
54755481
fallbackKeys,
54765482
remainingEnabledProviders,
54775483
).APIKey(deletedProvider.Provider) != "" {
54785484
return false
54795485
}
5480-
return remainingTotal == 0 ||
5481-
(deletedProvider.Enabled && remainingEnabledInFamily == 0)
5486+
return remainingTotal == 0
54825487
}
54835488

54845489
func filterUserVisibleChatModelConfigs(
54855490
configs []database.ChatModelConfig,
54865491
enabledProviders []database.ChatProvider,
54875492
deploymentKeys chatprovider.ProviderAPIKeys,
54885493
) []database.ChatModelConfig {
5494+
configs = slices.Clone(configs)
54895495
defaultKeys := defaultChatProviderAPIKeys(deploymentKeys, enabledProviders)
54905496
enabledProviderByID := make(map[uuid.UUID]database.ChatProvider, len(enabledProviders))
54915497
for _, provider := range enabledProviders {

‎site/src/pages/AgentsPage/components/ChatModelAdminPanel/ModelForm.tsx‎

Lines changed: 40 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@ import {
66
InfoIcon,
77
PencilIcon,
88
} from "lucide-react";
9-
import { type FC, useEffect, useState } from "react";
9+
import { type FC, useEffect, useMemo, useState } from "react";
1010
import * as Yup from "yup";
1111
import type * as TypesGen from "#/api/typesGenerated";
1212
import { Button } from "#/components/Button/Button";
@@ -62,13 +62,16 @@ const getInitialSelectedConfigId = (
6262
editingModel: TypesGen.ChatModelConfig | undefined,
6363
selectedProviderConfigs: readonly TypesGen.ChatProviderConfig[],
6464
): string => {
65-
if (editingModel?.provider_config_id) {
66-
const boundConfig = selectedProviderConfigs.find(
67-
(config) => config.id === editingModel.provider_config_id,
68-
);
69-
if (boundConfig) {
70-
return boundConfig.id;
65+
if (editingModel) {
66+
if (editingModel.provider_config_id) {
67+
const boundConfig = selectedProviderConfigs.find(
68+
(config) => config.id === editingModel.provider_config_id,
69+
);
70+
if (boundConfig) {
71+
return boundConfig.id;
72+
}
7173
}
74+
return "";
7275
}
7376
if (selectedProviderConfigs.length === 1) {
7477
return selectedProviderConfigs[0].id;
@@ -141,14 +144,36 @@ export const ModelForm: FC<ModelFormProps> = ({
141144
const [showProviderConfig, setShowProviderConfig] = useState(false);
142145
const [confirmingDelete, setConfirmingDelete] = useState(false);
143146
const selectedProviderConfigs = selectedProviderState?.providerConfigs ?? [];
147+
const configSignature = useMemo(
148+
() =>
149+
selectedProviderConfigs
150+
.map((config) => config.id)
151+
.sort()
152+
.join(","),
153+
[selectedProviderConfigs],
154+
);
155+
const selectableConfigs = useMemo(() => {
156+
return selectedProviderConfigs.filter(
157+
(config) =>
158+
config.enabled || editingModel?.provider_config_id === config.id,
159+
);
160+
}, [selectedProviderConfigs, editingModel?.provider_config_id]);
144161
const [selectedConfigId, setSelectedConfigId] = useState<string>(() =>
145-
getInitialSelectedConfigId(editingModel, selectedProviderConfigs),
162+
getInitialSelectedConfigId(editingModel, selectableConfigs),
146163
);
164+
// Biome cannot infer that configSignature is the intended stable proxy
165+
// for selectedProviderConfigs, so suppress exhaustive-deps here.
166+
// biome-ignore lint/correctness/useExhaustiveDependencies: configSignature intentionally gates resets on config ID changes.
147167
useEffect(() => {
148-
setSelectedConfigId(
149-
getInitialSelectedConfigId(editingModel, selectedProviderConfigs),
150-
);
151-
}, [editingModel, selectedProviderConfigs]);
168+
const currentStillValid =
169+
selectedConfigId !== "" &&
170+
selectedProviderConfigs.some((config) => config.id === selectedConfigId);
171+
if (!currentStillValid) {
172+
setSelectedConfigId(
173+
getInitialSelectedConfigId(editingModel, selectableConfigs),
174+
);
175+
}
176+
}, [editingModel, configSignature]);
152177
const canManageModels = Boolean(
153178
selectedProviderState &&
154179
selectedProviderConfigs.length > 0 &&
@@ -226,7 +251,7 @@ export const ModelForm: FC<ModelFormProps> = ({
226251
if (!selectedProviderState || selectedProviderConfigs.length === 0) {
227252
return;
228253
}
229-
if (selectedProviderConfigs.length > 1 && !selectedConfigId) {
254+
if (selectableConfigs.length > 1 && !selectedConfigId) {
230255
return;
231256
}
232257

@@ -275,7 +300,7 @@ export const ModelForm: FC<ModelFormProps> = ({
275300
const hasFieldErrors =
276301
Object.keys(modelConfigFormBuildResult.fieldErrors).length > 0;
277302
const requiresConfigSelection =
278-
!isEditing && selectedProviderConfigs.length > 1 && !selectedConfigId;
303+
!isEditing && selectableConfigs.length > 1 && !selectedConfigId;
279304
const defaultModelDisableGuard = isDefaultModel && form.values.enabled;
280305

281306
// ── Provider select (shared across all form states) ───────
@@ -340,7 +365,7 @@ export const ModelForm: FC<ModelFormProps> = ({
340365
No configuration
341366
</SelectItem>
342367
)}
343-
{selectedProviderConfigs.map((config) => (
368+
{selectableConfigs.map((config) => (
344369
<SelectItem key={config.id} value={config.id}>
345370
{config.display_name ||
346371
config.base_url ||

0 commit comments

Comments
 (0)