feat(tui): support configurable DeepSeek base URL in /config#1967
feat(tui): support configurable DeepSeek base URL in /config#1967cyq1017 wants to merge 4 commits into
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces support for viewing and configuring a base_url for DeepSeek-compatible endpoints, including persistence logic and unit tests. However, the reviewer noted that session-only overrides are currently non-functional because they do not update the application state. Additionally, the implementation for retrieving the setting relies on reloading the configuration from disk, which is inefficient and may ignore CLI-specified paths. Finally, the new setting needs to be added to the /settings display output for visibility.
| return CommandResult::message(format!( | ||
| "base_url = {value} (session only; use --save to persist)" | ||
| )); |
There was a problem hiding this comment.
The session-only override for base_url is currently a no-op. While the command returns a success message, it does not update the app state or the active DeepSeekClient. Consequently, the application will continue to use the previously configured base URL for the remainder of the session. To fix this, the base_url should be stored in the App struct and respected when creating or using the LLM client.
| let config = match Config::load(None, None) { | ||
| Ok(config) => config, | ||
| Err(err) => { | ||
| return CommandResult::error(format!("Failed to load config: {err}")); | ||
| } | ||
| }; | ||
| Some(config.deepseek_base_url()) |
There was a problem hiding this comment.
Retrieving the base_url by reloading the configuration from disk is inconsistent with other settings in this function, which typically use the runtime state from the app instance. This approach ignores any session-only overrides and performs unnecessary file I/O. Furthermore, Config::load(None, None) might load the wrong file if the application was started with a custom configuration path via CLI arguments.
| "base_url", | ||
| "HTTP base URL for DeepSeek-compatible endpoints.", | ||
| ), |
There was a problem hiding this comment.
While base_url has been added to available_settings (enabling it in /help config and /set usage), it is missing from the Settings::display method. This means it won't be visible when a user runs the /settings command. For consistency, it should be included in the display output, even if it is stored in config.toml rather than settings.toml.
|
Stewardship note: #1919 is now milestoned to v0.8.46 and this PR is the narrow candidate for persisted DeepSeek base_url config. I left the broader OpenAI-compatible path/provider parser questions in #1874 and #1978 so this PR can stay small. Review focus: persisted config semantics, restart/session messaging, and not accidentally promising hot reload. |
Summary
Testing
Closes #1919