UI: query provider for supported models #4270
Conversation
There was a problem hiding this comment.
Nice works well!
Couple of nits:
- maybe we want to add type to search also to provider dropdown label to be consistent?
- Currently, 500 is always returned if the provider fails, even for known/expected failures (e.g., invalid API key). Can we distinguish between transient errors and permanent misconfigurations?
Signed-off-by: Angela Ning <aning@squareup.com>
Signed-off-by: Angela Ning <aning@squareup.com>
DOsinga
left a comment
DOsinga
left a comment
There was a problem hiding this comment.
thanks for getting this in, but I think we can tighten up the code a bit more here
| 'X-Secret-Key': await window.electron.getSecretKey(), | ||
| }, | ||
| }); | ||
| return response.data || []; |
There was a problem hiding this comment.
this should use the generated API. you should never have to call getSecretKey
| }; | ||
| } | ||
| }); | ||
| const results = await Promise.all(modelPromises); |
There was a problem hiding this comment.
this doesn't seem ideal; if we wanted to get all the models for all the providers, we should just make that a flag on the getProviders method; the server can do that much more efficient.
but really, since we already split this up into selecting a provider first and then list the models, why not call this just in time? we have 28 providers, so that would avoid this mini DOS attack on our server.
| if (error) { | ||
| errors.push(error); | ||
| // Fallback to metadata known_models on error | ||
| if (p.metadata.known_models && p.metadata.known_models.length > 0) { |
There was a problem hiding this comment.
why do we have this fall back twice? also I don't think known_models is optional here, so we shouldn't need to check all this
| // Log errors if any providers failed (don't show to user) | ||
| if (errors.length > 0) { | ||
| console.error('Provider model fetch errors:', errors); | ||
| } |
There was a problem hiding this comment.
why do we have this complex machinery to keep track of the errors, only to log them here and then do nothing about them?
| } | ||
| } else if (models && models.length > 0) { | ||
| groupedOptions.push({ | ||
| options: models.map((m) => ({ value: m, label: m, provider: p.name })), |
Signed-off-by: Angela Ning <aning@squareup.com> Signed-off-by: Alex Rosenzweig <arosenzweig@squareup.com>
Signed-off-by: Angela Ning <aning@squareup.com> Signed-off-by: Dorien Koelemeijer <dkoelemeijer@squareup.com>
The UI queries the providers for the list of supported models, instead of using our hard-coded lists.
fixes #3645
Screen.Recording.2025-08-17.at.10.44.26.PM.mov