fix: provider endpoint and mux section #275
Conversation
Pull Request Test Coverage Report for Build 13195998776Details
💛 - Coveralls |
src/features/workspace/components/workspace-preferred-model.tsx
Outdated
Show resolved
Hide resolved
| @@ -27,3 +28,23 @@ export function isProviderType(value: unknown): value is ProviderType { | |||
| export function isProviderAuthType(value: unknown): value is ProviderAuthType { | |||
| return Object.values(ProviderAuthType).includes(value as ProviderAuthType); | |||
| } | |||
|
|
|||
| export function getProviderEndpointByAuthType(provider_type: ProviderType) { | |||
There was a problem hiding this comment.
I don't think this can/should be fixed in this PR, but just to raise the point: should this mapping/data not come from BE instead? I would like to avoid hardcoding the same thing in multiple different places. Otherwise there might be problems when we change the url for one of the providers in BE but not in FE or vice versa. This could be a problem since the number of providers keeps growing...
There was a problem hiding this comment.
well, this is only a prefilled text field for us. The BE doesn't have this logic cause it is up to the user specify the endpoint url...previously the provider endpoint field was empty, but correctly Dan and Luke pointed out that we can prefilled the url that are hardcoded for the providers
| .with(ProviderType.OPENAI, () => ProviderAuthType.API_KEY) | ||
| .with(ProviderType.ANTHROPIC, () => ProviderAuthType.API_KEY) | ||
| .with(ProviderType.OPENROUTER, () => ProviderAuthType.API_KEY) | ||
| .otherwise(() => ProviderAuthType.NONE); |
There was a problem hiding this comment.
should this one not also be exhaustive instead? and the same point, not in the scope of this PR, but if there is hardcoded "data" or mapping should be done in BE, unless it's something that is 100% only relevant in the dashboard 🤔
There was a problem hiding this comment.
the logic here is return api_key for those provider type, for the other return none, for that reason is correct to use otherwise rather than exhaustive. It will be none for the rest of the provider type, so duplicated a lot.
Also this is for prefilled fields in the form, so a UI logic, an helper, so the BE doesn't have this logic, they have only a validation in case of missing api_key for a provider
| usePreferredModelWorkspace(workspaceName); | ||
| const { mutateAsync } = useMutationPreferredModelWorkspace(); | ||
| const { data: providerModels = [] } = useModelsData(); | ||
| const { model, provider_id } = preferredModel; | ||
| const isModelsEmpty = !isPending && providerModels.length === 0; |
There was a problem hiding this comment.
nit pick, the naming is slightly questionable as isPending === true does not means models is empty. But I don't have a perfect suggestion so feel free to ignore the comment 🤷♂️
There was a problem hiding this comment.
🤔 sorry i don't understand, it is checking boht isPending and the length
auth_type,api_keyupdateauth_typeand provider endpointsorry 🙏 , I am still missing tests coverage, but after this PR I am going to add them.
Kapture.2025-02-06.at.21.32.02.mp4
Providers exist
No providers
