Skip to content

web: make the per-provider model dropdown visible without a PR - #78

Merged
tarekziade merged 2 commits into
mainfrom
fix/provider-model-dropdown-visibility
Jul 28, 2026
Merged

web: make the per-provider model dropdown visible without a PR#78
tarekziade merged 2 commits into
mainfrom
fix/provider-model-dropdown-visibility

Conversation

@sayakpaul

Copy link
Copy Markdown
Member

#59 shipped a per-provider model dropdown, but it isn't visible in the deployment: the Model field is still a free-text box on a freshly loaded New review page.

Why

Two independent reasons:

  1. The dropdown was gated on the PR field. For every provider except HF, updateModelControl() only swapped in the dropdown once the PR input parsed into an owner/repo and /reviews/models had returned a non-empty list for the matching config. With an empty PR field — the state in the screenshot — it always fell through to showModelInput().
  2. Anthropic model listing failed anyway. ChatCompletionClient.list_models() hits {base}/v1/models. Only /v1/chat/completions is Anthropic's OpenAI shim; /v1/models is the native route and rejects requests without an anthropic-version header. So even after typing a PR, the Anthropic list came back as an error and the form kept the text box.

What changed

  • New GET /llm-options/models?provider=X. Picks any provider config the caller is authorized for on that provider (repo_pattern ignored — the catalogue is the same whichever key lists it) and lists models server-side, so the key still never reaches the browser. Always 200: {models: [], error?} when there's no config or the /models route fails.
  • store.find_provider_config_for_provider() backs it — same allowed_users / allowed_orgs authorization as find_provider_config, without the repo match.
  • index.js populates the dropdown from that endpoint on load and on provider change, cached per provider. When a PR later resolves, the repo-matched list from /reviews/models takes precedence — that config is the one that will actually serve the review. Free-text box remains the fallback whenever no list can be had.
  • anthropic-version: 2023-06-01 is sent on requests to api.anthropic.com. The shim ignores it, the native route requires it.
  • /reviews/models and the new endpoint now share one _models_for_provider_config() helper instead of duplicating the list-and-swallow-errors block.

Verification

Ran the app locally against a fake OpenAI-compatible /models endpoint and drove index.html + index.js in jsdom:

on load (empty PR):          provider=custom dropdown=true options=[fake-a,fake-b] model=fake-a
after typing a matching PR:  provider=custom dropdown=true options=[fake-a,fake-b] hint="Using custom (via acme/*)."
after switching to a provider with no config: dropdown=false (free-text fallback)

New tests cover the endpoint (lists without a repo, empty without an authorized config, 400 on an unknown provider, error surfaced as a verdict), the store lookup's authorization, and the Anthropic header. Full suite passes (tests/test_app.py has a pre-existing import failure on main too).

🤖 Generated with Claude Code

PR #59 upgraded the Model field to a dropdown for keyed providers, but only
once the PR field parsed into an owner/repo that matched a provider config —
so a freshly loaded /new-review page (empty PR, Anthropic selected) always
showed the free-text box, which is what the deployment looks like.

Add GET /llm-options/models?provider=X: it picks any provider config the
caller is authorized for (repo_pattern ignored) and lists that endpoint's
models server-side, so the key still never reaches the browser. The form
populates from it on load and on provider change; once a PR resolves, the
repo-matched list from /reviews/models takes precedence, since that config
is the one that will actually serve the review.

Also send anthropic-version on requests to api.anthropic.com. /v1/models is
Anthropic's native route (only /v1/chat/completions is the OpenAI shim) and
rejects requests without it, so listing Anthropic models failed even with a
matching repo.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@sayakpaul

Copy link
Copy Markdown
Member Author

@askserge can you review the PR?

@sergereview sergereview Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🤗 Serge says:

This is a clean, well-tested fix that addresses both root causes described in the PR: the model dropdown is now populated on page load via a new provider-wide endpoint, and Anthropic's native /v1/models route receives the required anthropic-version header. The server-side listing keeps API keys out of the browser, authorization logic is consistent with existing patterns, and the frontend gracefully falls back to a free-text input when no catalogue can be fetched.

serge v0.1.0 · model: moonshotai/Kimi-K2.6 · 1 LLM turn · 0 tool calls · 27.3s · 10119 in / 4202 out tokens

@sayakpaul
sayakpaul requested a review from tarekziade July 28, 2026 09:01
Comment thread reviewbot/webapp.py Outdated
api_base, matched["api_key"], bill_to=bill_to, stream=False
)
body["models"] = client.list_models()
except HTTPException:

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This is not fully accurate as an HTTPException could be other reasons. Also, we should not expose the TB to the web to avoid leaking possible sensitive data and standardize a bit the error across all possible errors:

I suggest a single message "Could not retrieve list of models" and another field error_code with the HTTPException status code, and just logging.error for the traceback

Every listing failure now returns the same "Could not retrieve list of
models" message plus an error_code (the HTTPException status, else 500).
The exception text — which can carry the provider endpoint's own error
detail — is logged instead of returned.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@sayakpaul
sayakpaul requested a review from tarekziade July 28, 2026 09:26

@tarekziade tarekziade left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks!

@tarekziade
tarekziade merged commit 9cd27d1 into main Jul 28, 2026
3 checks passed
@tarekziade
tarekziade deleted the fix/provider-model-dropdown-visibility branch July 28, 2026 09:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants