feat(gui): add provider model loader#407
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR adds a free-provider directory with access metadata, trusted live/static model discovery, normalized model-list parsing, and a redesigned provider catalog and setup modal with model selection, cancellation, responsive layouts, icons, and localized strings. ChangesProvider directory and registry
Model discovery backend
Provider catalog UI
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 60046f85af
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const invalidateDiscoveryRequest = () => { | ||
| discoveryRequestRef.current += 1; | ||
| discoveryAbortRef.current?.abort(); | ||
| discoveryAbortRef.current = null; | ||
| setDiscoveryBusy(false); |
There was a problem hiding this comment.
When a user runs Discover models and then changes the endpoint/base URL/API key/adapter, all of those handlers call invalidateDiscoveryRequest(), but this helper only aborts pending fetches and leaves discoveredModels, selectedModels, and discoverySource intact. The stale list remains visible and submit() can persist that old selectedModels allowlist for the new endpoint without another discovery, so models from one regional/provider endpoint can be saved against a different one. Clear the discovered model state whenever invalidating due to provider input changes.
Useful? React with 👍 / 👎.
| const allowsModelsEnvelope = (prov.adapter === "google" && (prov.googleMode ?? "ai-studio") === "ai-studio") | ||
| || new URL(url).pathname.replace(/\/+$/, "").endsWith("/api/tags"); | ||
| const items = parseProviderModelsApiItems(json, allowsModelsEnvelope); |
There was a problem hiding this comment.
For Google AI Studio, the /v1beta/models envelope includes resources whose supported methods are embedding-only, but this new path accepts every name and exposes it as a chat model. The Google adapter later calls .../models/${id}:generateContent, so selecting an embedding-only id such as text-embedding-004 from live discovery saves a model that fails every chat request. Filter Google rows to models supporting generateContent/stream before adding them to the catalog or discovery response.
Useful? React with 👍 / 👎.
| const provider = { | ||
| ...providerConfigSeed(entry), |
There was a problem hiding this comment.
If a user changes the visible Adapter field before clicking Discover models, this route still rebuilds discovery from providerConfigSeed(entry) and never copies supplied.adapter, while the later /api/providers save path does persist the user's edited adapter. That lets discovery succeed against the preset transport (for example Google) and then save the same selected/default models against a different transport (for example openai-chat), where subsequent chat requests fail. Either reject adapter overrides for presets in the discover flow or run discovery with the same adapter that will be saved.
Useful? React with 👍 / 👎.
| const { resolveModelsAuthToken, buildModelsRequest } = await import("../../oauth"); | ||
| const apiKey = await resolveModelsAuthToken(presetId, provider); | ||
| const request = buildModelsRequest(provider, apiKey, presetId); | ||
| const modelsDestinationError = await providerDestinationResolvedError(presetId, { baseUrl: request.url }); |
There was a problem hiding this comment.
For presets that allow custom endpoints, such as Qwen Cloud, a user can enter a local/private compatible endpoint and check allowPrivateNetwork; the first destination check accepts that provider, but the second check against the derived /models URL drops allowPrivateNetwork. Discovery is therefore rejected even though the same provider can be saved and routed with the opt-in. Pass the provider's allowPrivateNetwork through this models-URL check as well.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/server/management/provider-routes.ts (1)
268-323: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
/api/providers/teststill allows fetch redirects — inconsistent with the SSRF hardening this PR just added.This PR adds
redirect: "error"to the discover endpoint's fetch (line 476) and tofetchProviderModels(provider-fetch.ts:368) specifically to stop a redirect from bypassing the destination-policy (SSRF) check performed on the pre-redirect URL. The connectivity-test endpoint at Line 295 does the exact same thing — build a/modelsrequest from a provider'sbaseUrlviabuildModelsRequestand fetch it with the caller-supplied credential — but keeps the defaultredirect: "follow"and never re-validates the destination at request time. A provider'sbaseUrlis destination-checked only when it's saved (POST/PATCH/api/providers); a DNS-rebind or a malicious upstream 3xx at test time can still redirect this fetch into a private/metadata address.🛡️ Suggested fix
const { url: modelsUrl, headers } = buildModelsRequest(prov, apiKey, name); const started = Date.now(); try { - const res = await fetch(modelsUrl, { headers, signal: AbortSignal.timeout(8000) }); + const res = await fetch(modelsUrl, { headers, redirect: "error", signal: AbortSignal.timeout(8000) });As per path instructions for
src/**: "Flag ... changes that bypass the shared routing/config layers" — this endpoint bypasses the sameproviderDestinationResolvedErrorre-check the sibling discover/fetch paths now perform.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/server/management/provider-routes.ts` around lines 268 - 323, Update the fetch call in the /api/providers/test handler to use redirect: "error", matching the SSRF-hardening behavior of the sibling provider-fetch and discover paths. Preserve the existing timeout, headers, response handling, and error response behavior while preventing upstream redirects from being followed.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@gui/src/i18n/de.ts`:
- Around line 1002-1036: Update the German locale export to import the TKey type
from en.ts and type its translations as Record<TKey, string>, removing the local
TKey = keyof typeof de definition. Preserve the existing German translation
entries while ensuring missing or extra keys are checked against the English
locale key set.
In `@gui/src/styles/provider-catalog.css`:
- Around line 12-19: Define a shared --catalog-head-h custom property for the
modal header height, set it to 62px in the default styles and 56px in the mobile
media query, then reuse it in both .provider-loader-card > .modal-head
min-height and .provider-catalog height calculations. Remove the duplicated
literal height values while preserving the existing responsive behavior.
---
Outside diff comments:
In `@src/server/management/provider-routes.ts`:
- Around line 268-323: Update the fetch call in the /api/providers/test handler
to use redirect: "error", matching the SSRF-hardening behavior of the sibling
provider-fetch and discover paths. Preserve the existing timeout, headers,
response handling, and error response behavior while preventing upstream
redirects from being followed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 74d4f98e-5638-440a-aa76-14ec2794b759
📒 Files selected for processing (24)
gui/src/components/AddProviderModal.tsxgui/src/components/provider-catalog/ProviderCatalog.tsxgui/src/components/provider-catalog/provider-presets.tsgui/src/i18n/de.tsgui/src/i18n/en.tsgui/src/i18n/ja.tsgui/src/i18n/ko.tsgui/src/i18n/ru.tsgui/src/i18n/zh.tsgui/src/provider-icons.tsgui/src/styles/provider-catalog.cssgui/tests/provider-catalog.test.tsgui/tests/provider-discovery-race.test.tsxsrc/codex/catalog/provider-fetch.tssrc/oauth/index.tssrc/providers/derive.tssrc/providers/free-directory.tssrc/providers/registry.tssrc/server/management/provider-routes.tstests/google-models-listing.test.tstests/provider-directory-routing.test.tstests/provider-free-directory.test.tstests/provider-model-envelope.test.tstests/provider-registry-parity.test.ts
| "modal.catalogModes": "Zugriffsarten der Anbieter", | ||
| "modal.accessGroups": "Kostenlose Zugriffsgruppen", | ||
| "modal.providerDirectory": "Anbieterverzeichnis", | ||
| "modal.searchCount": "{count} Anbieter durchsuchen", | ||
| "modal.group.all": "Alle Anbieter", | ||
| "modal.group.recurring-or-keyless": "Regelmäßig / ohne Schlüssel", | ||
| "modal.group.recurring-uncapped": "Kein öffentliches Token-Limit", | ||
| "modal.group.recurring-credit": "Regelmäßiges Guthaben", | ||
| "modal.group.signup-credit": "Startguthaben", | ||
| "modal.group.existing": "Vorhandene kostenlose & lokale", | ||
| "modal.paidHint": "Kostenpflichtige Anbieter und Anbieter mit einmaligem Startguthaben durchsuchen.", | ||
| "modal.selectProvider": "Anbieter auswählen", | ||
| "modal.selectProviderHint": "Wähle einen Anbieter, um die Einrichtung anzusehen und Modelle zu suchen.", | ||
| "modal.badge.recurring": "Regelmäßig kostenlos", | ||
| "modal.badge.reference": "Referenz", | ||
| "modal.badge.experimental": "Experimentell", | ||
| "modal.badge.supported": "Unterstützt", | ||
| "modal.verification.official": "Offizielle Dokumentation", | ||
| "modal.verification.primary": "Primärquelle", | ||
| "modal.verification.unverified": "Nicht verifiziert", | ||
| "modal.providerDocs": "Anbieterdokumentation", | ||
| "modal.referenceExplanation": "Dieser Eintrag dient nur als Referenz. Automatische Aufrufe und die Modellsuche bleiben deaktiviert, bis ein dokumentierter Transportweg verifiziert wurde.", | ||
| "modal.modelDiscovery": "Modellsuche", | ||
| "modal.modelDiscoveryHint": "Verfügbare Modelle für diese Einrichtung suchen.", | ||
| "modal.discoverModels": "Modelle suchen", | ||
| "modal.discovering": "Suche läuft…", | ||
| "modal.modelsLive": "Live-Antwort des Anbieters", | ||
| "modal.modelsStatic": "Statischer Katalog", | ||
| "modal.discoveredModels": "Gefundene Modelle", | ||
| "modal.discoveryFailed": "Für diese Einrichtung konnten keine Modelle gefunden werden.", | ||
| "modal.backToDirectory": "Zurück zum Anbieterverzeichnis", | ||
| "modal.setupReviewProvider": "Einrichtungsdokumentation für {label} prüfen.", | ||
| "modal.setupReferenceStep": "Nutze diesen Eintrag als Einrichtungshilfe, bis verifizierte Aufrufunterstützung verfügbar ist.", | ||
| "modal.setupKeylessStep": "Bestätige den Endpunkt unten; ein API-Schlüssel ist optional.", | ||
| "modal.setupDiscoverStep": "Modelle suchen und ein Standardmodell auswählen.", |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
German locale is the only one not type-checked against en.ts's key set.
All 37 new keys are present and consistent here (verified against ja/ko/ru/zh), so nothing is broken today. But de.ts exports its own TKey = keyof typeof de (line 1163) instead of import type { TKey } from "./en" + Record<TKey, string> like every other locale file. That means a future key added to en.ts but missed in de.ts won't be caught at compile time here, unlike ja/ko/ru/zh.
♻️ Suggested fix
-export const de = {
+import type { TKey } from "./en";
+
+export const de: Record<TKey, string> = {
"nav.dashboard": "Übersicht",
...
-} as const;
-
-export type TKey = keyof typeof de;
+};🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@gui/src/i18n/de.ts` around lines 1002 - 1036, Update the German locale export
to import the TKey type from en.ts and type its translations as Record<TKey,
string>, removing the local TKey = keyof typeof de definition. Preserve the
existing German translation entries while ensuring missing or extra keys are
checked against the English locale key set.
| .provider-loader-card > .modal-head { | ||
| min-height: 62px; | ||
| margin: 0; | ||
| padding: 12px 18px; | ||
| border-bottom: 1px solid var(--border); | ||
| } | ||
|
|
||
| .provider-catalog { display: flex; flex-direction: column; min-height: 0; height: calc(100% - 62px); } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Modal-head height (62px/56px) is duplicated as a magic number across two rules per breakpoint.
.provider-loader-card > .modal-head { min-height: 62px; ... } / .provider-catalog { height: calc(100% - 62px); } (and the 56px mobile pair at lines 102-103) must be kept in sync by hand; a future tweak to one is easy to forget in the other, silently breaking the catalog's internal scroll height.
Consider a CSS custom property, e.g. --catalog-head-h: 62px; (56px in the mobile media query), referenced from both rules.
Also applies to: 100-103
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@gui/src/styles/provider-catalog.css` around lines 12 - 19, Define a shared
--catalog-head-h custom property for the modal header height, set it to 62px in
the default styles and 56px in the mobile media query, then reuse it in both
.provider-loader-card > .modal-head min-height and .provider-catalog height
calculations. Remove the duplicated literal height values while preserving the
existing responsive behavior.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
60046f8 to
9638108
Compare
Summary
Dependencies
Merge after #405 and #406. Their commits disappear from this diff as prerequisites land.
Tests
Summary by CodeRabbit