fix: ModelID cannot be saved and refactor ModelPicker #1122
Conversation
System233
requested review from
ColemanRoo,
cte,
mrubens and
stea9499
as code owners
|
dosubot
bot
added
size:XXL
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
System233
force-pushed
the
patch-model-picker
branch
from
be47c8c to
f7e4599
Compare
System233
changed the title
System233
force-pushed
the
patch-model-picker
branch
from
f7e4599 to
8aced86
Compare
System233
changed the title
|
@System233 thank you for the PR! There's a lot going on in here though, and I'm unsure about some of the UX changes. Would it be possible to separate out the bugfix into its own PR, or highlight which parts fix the bug so I can try? Thank you! |
System233
force-pushed
the
patch-model-picker
branch
from
8aced86 to
f5c4b01
Compare
System233
force-pushed
the
patch-model-picker
branch
from
f5c4b01 to
4897500
Compare
I have split the commit.
|
| switch (message.type) { | ||
| case "ollamaModels": | ||
| { | ||
| const newModels = message.ollamaModels ?? [] | ||
| setOllamaModels(newModels) | ||
| } | ||
| break | ||
| case "lmStudioModels": | ||
| { | ||
| const newModels = message.lmStudioModels ?? [] | ||
| setLmStudioModels(newModels) | ||
| } | ||
| break | ||
| case "vsCodeLmModels": | ||
| { | ||
| const newModels = message.vsCodeLmModels ?? [] | ||
| setVsCodeLmModels(newModels) | ||
| } | ||
| break | ||
| case "glamaModels": { | ||
| const updatedModels = message.glamaModels ?? {} | ||
| setGlamaModels({ | ||
| [glamaDefaultModelId]: glamaDefaultModelInfo, // in case the extension sent a model list without the default model | ||
| ...updatedModels, | ||
| }) | ||
| break | ||
| } | ||
| case "openRouterModels": { | ||
| const updatedModels = message.openRouterModels ?? {} | ||
| setOpenRouterModels({ | ||
| [openRouterDefaultModelId]: openRouterDefaultModelInfo, // in case the extension sent a model list without the default model | ||
| ...updatedModels, | ||
| }) | ||
| break | ||
| } | ||
| case "openAiModels": { | ||
| const updatedModels = message.openAiModels ?? [] | ||
| setOpenAiModels(Object.fromEntries(updatedModels.map((item) => [item, openAiModelInfoSaneDefaults]))) | ||
| break | ||
| } | ||
| case "unboundModels": { | ||
| const updatedModels = message.unboundModels ?? {} | ||
| setUnboundModels(updatedModels) | ||
| break | ||
| } | ||
| case "requestyModels": { | ||
| const updatedModels = message.requestyModels ?? {} | ||
| setRequestyModels({ | ||
| [requestyDefaultModelId]: requestyDefaultModelInfo, // in case the extension sent a model list without the default model | ||
| ...updatedModels, | ||
| }) | ||
| break | ||
| } |
There was a problem hiding this comment.
Handle all model list requests in ApiOptions
| <span style={{ fontWeight: 500 }}>API Key</span> | ||
| </VSCodeTextField> | ||
| <OpenAiModelPicker /> | ||
| <ModelPicker | ||
| apiConfiguration={apiConfiguration} | ||
| modelIdKey="openAiModelId" | ||
| modelInfoKey="openAiCustomModelInfo" | ||
| serviceName="OpenAI" | ||
| serviceUrl="https://platform.openai.com" | ||
| recommendedModel="gpt-4-turbo-preview" | ||
| models={openAiModels} | ||
| setApiConfigurationField={setApiConfigurationField} | ||
| defaultModelInfo={openAiModelInfoSaneDefaults} | ||
| errorMessage={errorMessage} | ||
| /> | ||
| <div style={{ display: "flex", alignItems: "center" }}> | ||
| <Checkbox |
There was a problem hiding this comment.
Since provider-specific ModelPicker (such as OpenAiModelPicker) does not have substantial new code, all provider-specific ModelPickers are deleted here and ModelPicker is used directly.
In addition, in the previous OpenAiModelPicker, openAiCustomModelInfo was incorrectly mapped to openAiModelInfo due to incorrect type constraints.
| const [customModelId, setCustomModelId] = useState("") | ||
| const [isCustomModel, setIsCustomModel] = useState(false) | ||
| const [open, setOpen] = useState(false) | ||
| const [value, setValue] = useState(defaultModelId) | ||
| const [isDescriptionExpanded, setIsDescriptionExpanded] = useState(false) | ||
| const prevRefreshValuesRef = useRef<Record<string, any> | undefined>() | ||
|
|
||
| const { apiConfiguration, [modelsKey]: models, onUpdateApiConfig, setApiConfiguration } = useExtensionState() | ||
|
|
||
| const modelIds = useMemo( | ||
| () => (Array.isArray(models) ? models : Object.keys(models)).sort((a, b) => a.localeCompare(b)), | ||
| [models], | ||
| ) | ||
| const modelIds = useMemo(() => Object.keys(models ?? {}).sort((a, b) => a.localeCompare(b)), [models]) | ||
|
|
||
| const { selectedModelId, selectedModelInfo } = useMemo( | ||
| () => normalizeApiConfiguration(apiConfiguration), | ||
| [apiConfiguration], | ||
| ) | ||
|
|
||
| const onSelectCustomModel = useCallback( | ||
| (modelId: string) => { | ||
| setCustomModelId(modelId) | ||
| const modelInfo = { id: modelId } | ||
| const apiConfig = { ...apiConfiguration, [configKey]: modelId, [infoKey]: modelInfo } | ||
| setApiConfiguration(apiConfig) | ||
| onUpdateApiConfig(apiConfig) | ||
| setValue(modelId) | ||
| setOpen(false) | ||
| setIsCustomModel(false) | ||
| }, | ||
| [apiConfiguration, configKey, infoKey, onUpdateApiConfig, setApiConfiguration], | ||
| ) | ||
|
|
||
| const onSelect = useCallback( | ||
| (modelId: string) => { | ||
| const modelInfo = Array.isArray(models) | ||
| ? { id: modelId } // For OpenAI models which are just strings | ||
| : models[modelId] // For other models that have full info objects | ||
| const apiConfig = { ...apiConfiguration, [configKey]: modelId, [infoKey]: modelInfo } | ||
| setApiConfiguration(apiConfig) | ||
| onUpdateApiConfig(apiConfig) | ||
| setValue(modelId) | ||
| setOpen(false) | ||
| const modelInfo = models?.[modelId] | ||
| setApiConfigurationField(modelIdKey, modelId) | ||
| setApiConfigurationField(modelInfoKey, modelInfo ?? defaultModelInfo) | ||
| }, | ||
| [apiConfiguration, configKey, infoKey, models, onUpdateApiConfig, setApiConfiguration], | ||
| [modelIdKey, modelInfoKey, models, setApiConfigurationField, defaultModelInfo], | ||
| ) | ||
|
|
||
| const debouncedRefreshModels = useMemo(() => { | ||
| return debounce(() => { | ||
| const message = refreshValues | ||
| ? { type: refreshMessageType, values: refreshValues } | ||
| : { type: refreshMessageType } | ||
| vscode.postMessage(message) | ||
| }, 100) | ||
| }, [refreshMessageType, refreshValues]) | ||
|
|
||
| useMount(() => { | ||
| debouncedRefreshModels() | ||
| return () => debouncedRefreshModels.clear() | ||
| }) | ||
|
|
||
| useEffect(() => { | ||
| if (!refreshValues) { | ||
| prevRefreshValuesRef.current = undefined | ||
| return | ||
| } | ||
|
|
||
| // Check if all values in refreshValues are truthy | ||
| if (Object.values(refreshValues).some((value) => !value)) { | ||
| prevRefreshValuesRef.current = undefined | ||
| return | ||
| if (apiConfiguration[modelIdKey] == null && defaultModelId) { | ||
| onSelect(defaultModelId) | ||
| } | ||
|
|
||
| // Compare with previous values | ||
| const prevValues = prevRefreshValuesRef.current | ||
| if (prevValues && JSON.stringify(prevValues) === JSON.stringify(refreshValues)) { | ||
| return | ||
| } | ||
|
|
||
| prevRefreshValuesRef.current = refreshValues | ||
| debouncedRefreshModels() | ||
| }, [debouncedRefreshModels, refreshValues]) | ||
|
|
||
| useEffect(() => setValue(selectedModelId), [selectedModelId]) |
There was a problem hiding this comment.
const { apiConfiguration, [modelsKey]: models, onUpdateApiConfig, setApiConfiguration } = useExtensionState()Previously, ModelPicker edited settings on the global ExtensionState/ExtensionStateContext, which prevented the modelID field from being saved.
The debounce and model list request related codes are useless here, and the custom model input function also introduces new UI errors, so I replaced it with an auto-complete combo box, and all problems were solved.
| <ComboboxItem key={model} value={model}> | ||
| {model} | ||
| </ComboboxItem> | ||
| ))} | ||
| </ComboboxContent> | ||
| </Combobox> | ||
|
|
||
| {errorMessage ? ( | ||
| <ApiErrorMessage errorMessage={errorMessage}> | ||
| <p | ||
| style={{ | ||
| fontSize: "12px", | ||
| marginTop: 3, | ||
| color: "var(--vscode-descriptionForeground)", | ||
| }}> | ||
| <span style={{ color: "var(--vscode-errorForeground)" }}> | ||
| <span style={{ fontWeight: 500 }}>Note:</span> Roo Code uses complex prompts and works best | ||
| with Claude models. Less capable models may not work as expected. | ||
| </span> | ||
| </p> | ||
| </ApiErrorMessage> |
There was a problem hiding this comment.
Displaying the error message directly below the modelID input box makes it easier for users to see.
TODO: Do not display a default, built-in defaultModelInfo for unknown models
| } | ||
| case "mcpServers": { | ||
| setMcpServers(message.mcpServers ?? []) | ||
| break |
There was a problem hiding this comment.
Further reduce the complexity of ExtensionStateContext, don’t stuff everything here.
| @@ -0,0 +1,6 @@ | |||
| import React from "react" | |||
There was a problem hiding this comment.
We should really figure out to get pure ESM modules to work with our jest configuration 🤔
There was a problem hiding this comment.
I tried it before, try again now
There was a problem hiding this comment.
@cte
After trying it, adding the following configuration to jest can pass the test. Although it is better than mock export, I am not sure whether hard coding the cjs path is a good idea:
moduleNameMapper: {
"\\.(css|less|scss|sass)$": "identity-obj-proxy",
"^vscrui$": "<rootDir>/src/__mocks__/vscrui.ts",
+ "^lucide-react$": "<rootDir>/node_modules/lucide-react/dist/cjs/lucide-react.js",
"^@vscode/webview-ui-toolkit/react$": "<rootDir>/src/__mocks__/@vscode/webview-ui-toolkit/react.ts",
"^@/(.*)$": "<rootDir>/src/$1",
},
cte
left a comment
cte
left a comment
There was a problem hiding this comment.
This is a huge improvement - thanks!
|
@System233 can you please reach out to me via email [email protected] or on discord #hrudolph ? |
4 participants
Description
The main changes for bug fixes are in f065f03
ModelPicker, allowing it to edit settings on theapiConfigurationprovided bySettingsView, avoiding direct editing ofExtensionStateContext.ExtensionStateContextto be centrally handled inApiOptions. The model list request events will be completely removed fromExtensionStateContextin a future commit to reduce the complexity ofExtensionStateContext.Commit 1a3b870 introduces the following user experience improvements:
The form validation error message is now displayed directly below the ModelID input field, making it more noticeable for users. Previously, the error message appeared after the model list and model configuration, requiring users to scroll down to see it. Additionally, since it was displayed alongside the similarly colored "Note", users often mistook the error message for part of the note and overlooked it.
When the form validation fails, an error message is shown on the Save button, and the button's border is highlighted in red (vscode-errorForeground) to indicate what the issue is.
The validation messages for
validateApiConfigurationandvalidateModelIdhave been consolidated. The functionality of these validations is very similar, and they can be merged in the future.Now:
Before:
Other notes:
The validation for
validateApiConfigurationis incomplete, as not all providers correctly validate thekeyandmodelId.TODO:
Type of change
How Has This Been Tested?
Checklist:
Additional context
Related Issues
Reviewers
Important
Fix and refactor
ModelPickerto improve snapshotting and add auto-complete feature.ModelPickernot being properly snapshotted and remove references toExtensionStateContext.openAiCustomModelInfoinApiOptions.tsx.ModelPickercomponent inModelPicker.tsx.OpenAiModelPicker,OpenRouterModelPicker,GlamaModelPicker,RequestyModelPicker, andUnboundModelPicker.ModelPickerusingComboboxinModelPicker.tsx.CustomModelinput handling inApiOptions.tsx.ModelPicker.test.tsxto test new auto-complete functionality and refactoredModelPickerbehavior.This description was created by
for be47c8ce4482a12ef21c1514d3a2d984a4a702be. It will automatically update as commits are pushed.