Skip to content

Conversation

lugnicca
Copy link

@lugnicca lugnicca commented Aug 23, 2025

Describe Your Changes

  • users can either type custom model IDs (original way) or select from a dropdown of available models fetched from the provider's /v1/models endpoint
  • automatically fetches models from provider APIs with caching
  • full keyboard navigation with arrow keys, Enter, Escape, PageUp/PageDown
  • more descriptive error messages for different API failure scenarios (401, 403, 404, etc.) when fetching models
  • API key is not mandatory for fetching models (Openrouter and HuggingFace)
modelselector.mp4

Fixes Issues

Self Checklist

  • Added relevant comments, esp in complex areas

Important

Introduces a ModelCombobox for selecting models from provider APIs with enhanced error handling and caching, updating the AddModel dialog and adding comprehensive tests.

  • Behavior:
    • Adds ModelCombobox component in ModelCombobox.tsx for selecting models from a dropdown fetched from /v1/models endpoint.
    • Supports keyboard navigation and displays error messages for API failures (401, 403, 404).
    • API key is optional for fetching models from Openrouter and HuggingFace.
  • Dialog Updates:
    • Replaces input with ModelCombobox in AddModel.tsx for model selection.
    • Fetches models using useProviderModels hook.
  • Hooks:
    • Introduces useProviderModels in useProviderModels.ts to fetch and cache models from provider APIs.
    • Caches models for 5 minutes and handles errors with specific messages.
  • Tests:
    • Adds tests for ModelCombobox in ModelCombobox.test.tsx.
    • Adds tests for useProviderModels in useProviderModels.test.ts.
    • Updates providers.test.ts to test new error handling in fetchModelsFromProvider.

This description was created by Ellipsis for aa568e6. You can customize this summary. It will automatically update as commits are pushed.

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Changes requested ❌

Reviewed everything up to aa568e6 in 1 minute and 59 seconds. Click for details.
  • Reviewed 1339 lines of code in 8 files
  • Skipped 0 files when reviewing.
  • Skipped posting 1 draft comments. View those below.
  • Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. web-app/src/services/providers.ts:222
  • Draft comment:
    In updateSettings, the conversion from 'controller_props' to 'controllerProps' uses a conditional to replace undefined values with ''. Consider using a nullish coalescing operator to handle both undefined and null, ensuring consistency with expected types.
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.

Workflow ID: wflow_WDRqMx6a4ICpwbfY

You can customize Ellipsis by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.

@louis-menlo
Copy link
Contributor

This is awesome!

@louis-menlo
Copy link
Contributor

It works great, but I found two edge cases during testing:

  1. When I select the Ollama provider without Ollama running, it shows a refresh button in the dialog. I then open Ollama and click the refresh button → the button disappears (no models are downloaded). After downloading a model from Ollama, the dropdown still shows an empty state and there is no refresh button.
    Expected behavior: the refresh button should always be visible.
CleanShot 2025-09-02 at 12 04 39@2x
  1. Following the same flow as above, but without opening Ollama. Instead, I switch to the Gemini provider and then go back. It shows an error message for Gemini even though I had selected the Ollama provider.
CleanShot 2025-09-02 at 12 05 01@2x

@louis-menlo louis-menlo self-assigned this Sep 2, 2025
@lugnicca
Copy link
Author

lugnicca commented Sep 2, 2025

It works great, but I found two edge cases during testing:

1. When I select the Ollama provider without Ollama running, it shows a refresh button in the dialog. I then open Ollama and click the refresh button → the button disappears (no models are downloaded). After downloading a model from Ollama, the dropdown still shows an empty state and there is no refresh button.
   Expected behavior: the refresh button should always be visible.
CleanShot 2025-09-02 at 12 04 39@2x
2. Following the same flow as above, but without opening Ollama. Instead, I switch to the Gemini provider and then go back.  It shows an error message for Gemini even though I had selected the Ollama provider.
CleanShot 2025-09-02 at 12 05 01@2x

Thanks for the feeback !

For the first issue, I added the refresh button in the input instead of inside the error message, useful for refreshing even when the cache is in place, and it covers the case for no models and errors. Let me know what you think !

image

For the second one, I managed to recreate the bug but with Ollama error showing on Gemini provider, it seems it was some kind of race condition that was showing the stale response on the UI for the wrong provider.
I added a simple ID guard that tracks the current request and ignores responses that don't match, it seems to have done the trick as I don't encounter the case anymore, let me know how it goes on your end !

Thanks again :)

@louis-menlo
Copy link
Contributor

louis-menlo commented Sep 5, 2025

Thanks @lucido-simon, please help me resolve the conflict so we can proceed with the merge

@lucido-simon
Copy link
Contributor

Thanks @lucido-simon, please help me resolve the conflict so we can proceed with the merge

I think you meant to ping @lugnicca

@louis-menlo
Copy link
Contributor

Ah yes, sorry @lucido-simon

Copy link
Contributor

@louis-menlo louis-menlo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@louis-menlo
Copy link
Contributor

Different behaviors for refreshing the model list

  1. Can successfully get the model list from the provider screen
CleanShot 2025-09-08 at 13 07 15@2x
  1. Can't get model list from model dropdown
CleanShot 2025-09-08 at 13 07 10@2x

@lugnicca
Copy link
Author

lugnicca commented Sep 8, 2025

Different behaviors for refreshing the model list

1. Can successfully get the model list from the provider screen
CleanShot 2025-09-08 at 13 07 15@2x
2. Can't get model list from model dropdown
CleanShot 2025-09-08 at 13 07 10@2x

Thanks for the feedback, seems i got confused with the new separation app/web and services used, fixed it !

Thanks for your patience !

@louis-menlo
Copy link
Contributor

Awesome, thanks @lugnicca

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

4 participants