fix: forward base URL overrides to active providers#1313
fix: forward base URL overrides to active providers#1313reidliu41 wants to merge 1 commit intoHmbown:mainfrom
Conversation
Route DEEPSEEK_BASE_URL through the active provider config instead of leaving self-hosted providers on their localhost defaults. This makes --base-url work for Ollama and vLLM while preserving provider-specific env overrides.
There was a problem hiding this comment.
Code Review
This pull request updates the apply_env_overrides function in crates/tui/src/config.rs to explicitly handle the DEEPSEEK_BASE_URL environment variable for several API providers, replacing a previous catch-all implementation. It also introduces a new test case to verify that these overrides correctly apply to self-hosted providers like Ollama and Vllm. Feedback suggests refactoring the repetitive logic used to update provider-specific base URLs to reduce code duplication and improve maintainability.
| ApiProvider::Openrouter => { | ||
| config | ||
| .providers | ||
| .get_or_insert_with(ProvidersConfig::default) | ||
| .openrouter | ||
| .base_url = Some(value); | ||
| } | ||
| ApiProvider::Novita => { | ||
| config | ||
| .providers | ||
| .get_or_insert_with(ProvidersConfig::default) | ||
| .novita | ||
| .base_url = Some(value); | ||
| } | ||
| ApiProvider::Fireworks => { | ||
| config | ||
| .providers | ||
| .get_or_insert_with(ProvidersConfig::default) | ||
| .fireworks | ||
| .base_url = Some(value); | ||
| } | ||
| ApiProvider::Sglang => { | ||
| config | ||
| .providers | ||
| .get_or_insert_with(ProvidersConfig::default) | ||
| .sglang | ||
| .base_url = Some(value); | ||
| } | ||
| ApiProvider::Vllm => { | ||
| config | ||
| .providers | ||
| .get_or_insert_with(ProvidersConfig::default) | ||
| .vllm | ||
| .base_url = Some(value); | ||
| } | ||
| ApiProvider::Ollama => { | ||
| config | ||
| .providers | ||
| .get_or_insert_with(ProvidersConfig::default) | ||
| .ollama | ||
| .base_url = Some(value); | ||
| } |
There was a problem hiding this comment.
There's a lot of repeated code here for handling different providers. You can refactor this to reduce duplication by using a match binding and another match to get the mutable provider config. This will make the code more maintainable if more providers are added in the future.
provider @ (ApiProvider::Openrouter
| ApiProvider::Novita
| ApiProvider::Fireworks
| ApiProvider::Sglang
| ApiProvider::Vllm
| ApiProvider::Ollama) => {
let providers = config.providers.get_or_insert_with(ProvidersConfig::default);
let provider_config = match provider {
ApiProvider::Openrouter => &mut providers.openrouter,
ApiProvider::Novita => &mut providers.novita,
ApiProvider::Fireworks => &mut providers.fireworks,
ApiProvider::Sglang => &mut providers.sglang,
ApiProvider::Vllm => &mut providers.vllm,
ApiProvider::Ollama => &mut providers.ollama,
_ => unreachable!(),
};
provider_config.base_url = Some(value);
}
Summary
Fixes provider base URL overrides for self-hosted providers.
deepseek --provider ollama --base-url ...anddeepseek --provider vllm --base-url ...were still resolving to the default localhost endpoints. The dispatcher forwards
--base-urlasDEEPSEEK_BASE_URL, but the TUI config loader only routed thatvalue into a few provider-specific config blocks. For Ollama and vLLM it landed
on the legacy root
base_url, which those providers do not read.This change routes
DEEPSEEK_BASE_URLinto the active provider's config block,so Ollama and vLLM honor
--base-urlcorrectly. Existing provider-specific envvars such as
OLLAMA_BASE_URLandVLLM_BASE_URLcontinue to work.fixes: #1308
Testing
cargo test --all-featurescargo fmt --all -- --checkcargo clippy --all-targets --all-featuresChecklist