refactor: unify model picker components #923
Conversation
- Extend ModelPicker to support OpenAI models and refreshValues - Refactor OpenAiModelPicker to use unified ModelPicker component - Add refreshValues support for Requesty API key
samhvw8
requested review from
ColemanRoo,
cte,
mrubens and
stea9499
as code owners
|
|
Wow, nice cleanup! |
| import { normalizeApiConfiguration } from "./ApiOptions" | ||
| import { ModelInfoView } from "./ModelInfoView" | ||
|
|
||
| interface ModelPickerProps { |
There was a problem hiding this comment.
Feel free to ignore, but Claude suggested doing this:
type ModelProvider = "glama" | "openRouter" | "unbound" | "requesty" | "openAi"
type ModelKeys<T extends ModelProvider> = `${T}Models`
type ConfigKeys<T extends ModelProvider> = `${T}ModelId`
type InfoKeys<T extends ModelProvider> = `${T}ModelInfo`
type RefreshMessageType<T extends ModelProvider> = `refresh${Capitalize<T>}Models`
interface ModelPickerProps<T extends ModelProvider = ModelProvider> {
defaultModelId: string
modelsKey: ModelKeys<T>
configKey: ConfigKeys<T>
infoKey: InfoKeys<T>
refreshMessageType: RefreshMessageType<T>
refreshValues?: Record<string, any>
serviceName: string
serviceUrl: string
recommendedModel: string
}
|
Found one bug: After entering my Requesty API key the model picker doesn't populate the models: Other than that the code looks good. |
- Add proper change detection for refreshValues using deep comparison - Increase debounce timing from 50ms to 100ms for better performance - Add proper error handling for missing Requesty API key - Fix apiConfiguration update to properly merge with existing state - Remove debug console logs
@cte i just fix this, and some another bug |
|
@cte it's ok now, but there is an problems that is openai compatible must need to implement /v1/models to work with us :(, let me fix |
done for fix custom model in openai compatible |
|
Nice, verified that the fix is working for me. One last thing. Let's change onInput={handleInputChange("requestyApiKey")}to onBlur={handleInputChange("requestyApiKey")}Otherwise the hook you added that reacts to |
- Add allowCustomModel prop to control custom model feature - Implement custom model input dialog with proper VSCode theming - Enable custom model support in OpenAiModelPicker - Fix z-index issues with modal dialog
Very unrelated question. If we have debouncing logic, would that set us up nicely to still use |
|
@cte done :D |
Seems less problematic with debouncing, but I think it makes sense to simply avoid unnecessary work (there's no reason to execute the hook while the user is typing). |
|
|
||
| prevRefreshValuesRef.current = refreshValues | ||
| debouncedRefreshModels() | ||
| }, [debouncedRefreshModels, refreshValues]) |
There was a problem hiding this comment.
Nitpick - adding JSON.stringify(refreshValues) to the dependecy array might help clean this up.
useEffect(() => {
if (!refreshValues) return;
debouncedRefreshModels();
}, [debouncedRefreshModels, JSON.stringify(refreshValues)]);I'm not 100% sure the ref is actually needed, but I'll defer to you.
4 participants
Description
Type of change
How Has This Been Tested?
Checklist:
Additional context
Related Issues
Reviewers
Important
Refactor to unify model picker components, extending
ModelPickerto support OpenAI models andrefreshValues.OpenAiModelPickerandRequestyModelPickernow use the unifiedModelPickercomponent.ModelPickerinModelPicker.tsxextended to support OpenAI models andrefreshValues.refreshValuessupport for Requesty API key inRequestyModelPicker.tsx.ModelPicker.tsx.This description was created by
for 26a315b. It will automatically update as commits are pushed.