chore: integrate backend API and remove hardcoded model options #2419
Conversation
evanliu048
changed the title
| self.model_name.as_deref().unwrap_or(&self.model_id) | ||
| } | ||
| } | ||
| impl<'de> Deserialize<'de> for ModelInfo { |
There was a problem hiding this comment.
Instead of implementing Deserialize manually, can we just change the name of the model field inside ConversationState? So instead we'd have:
/// Legacy, unused
/// Model explicitly selected by the user in this conversation state via `/model`.
#[serde(default, skip_serializing_if = "Option::is_none")]
pub model: Option<String>,
/// Model explicitly selected by the user in this conversation state via `/model`.
#[serde(default, skip_serializing_if = "Option::is_none")]
pub model_info: Option<ModelInfo>,
this way, we won't have to manually implemented Deserialize which is a bit strange
There was a problem hiding this comment.
Yes, I thought about doing it that way too! I just didn’t want to keep an extra pub model: Option field around since it’s effectively unused now and just there for backward compatibility. I’m fine to add it if we think keeping it for clarity and avoiding the manual Deserialize is worth the extra field. Let me know your thoughts!
There was a problem hiding this comment.
I think we can just remove the pub on it and keep it as an unused field, with a comment explaining why it's unused (for backwards compatibility) and to never add it for anything else going forward.
Think this is far better than a manual Deserialize impl - if the user loads a previous conversation, we just reset the active model to whatever the default is
There was a problem hiding this comment.
Makes sense — I dropped the manual Deserialize and keep a non-pub legacy model: Option<String> on ConversationState for backward compatibility. We’ll ignore it at runtime and fall back to the default model if no model_info is present
crates/chat-cli/src/cli/chat/mod.rs
Outdated
| .find(|m| m.model_id == model_id) | ||
| .map(|m| m.model_id.clone()) | ||
| }) | ||
| { |
There was a problem hiding this comment.
Can we print an error message to warn the user if their default model from the settings is missing? Since we're changing model names with this release, lots of people might have an incorrect name set up going forward
There was a problem hiding this comment.
Thanks for checking!
We didn’t change the model name in this release — the API still returns both the name and ID as claude-4-sonnet. In the past, if a user set an arbitrary value in the settings, it simply wouldn’t take effect and no warning was shown. Do we want to start notifying users about this now?
There was a problem hiding this comment.
But thanks for checking since I need to compare model name first!
There was a problem hiding this comment.
Oh in that case I suppose we don't need to add any notification, although it would still be a nice to have imo!
There was a problem hiding this comment.
Totally agree! Right now, if the user runs q chat --model with an invalid value, they do get a notification. It’s only when the value comes from the settings that there’s no notification — it just silently doesn’t take effect
3 participants
Issue #, if available:
Description of changes:
This PR removes the hardcoded
MODEL_OPTIONSand replaces it with a dynamic fetch via thelistAvailableModelsAPI. The result is cached for the duration of the CLI session to avoid redundant network calls and improve responsiveness. No need to map the model ID to a user-friendly name on the client side, as the backend will handle the mapping.By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.