agent: Fix summarization model being cleared by unrelated registry events (#52080)
## Context We were seeing lots of pending title generation, which should only happen if we don't have a summarization model. `handle_models_updated_event` unconditionally overwrote the thread's summarization model on every registry event, even with `None`. We should only setting if explicitly changed by the user or we haven't set it yet, and only if we actually have one. It is hard to reproduce this issue.. but I don't think this code was right in the first place anyway. ## Self-Review Checklist - [x] I've reviewed my own diff for quality, security, and reliability - [x] Unsafe blocks (if any) have justifying comments - [x] The content is consistent with the [UI/UX checklist](https://github.com/zed-industries/zed/blob/main/CONTRIBUTING.md#uiux-checklist) - [x] Tests cover the new/changed behavior - [x] Performance impact has been considered and is acceptable Release Notes: - N/A
This commit is contained in:
1 parent
591b3a0650
commit
03fc8dcb26
1 file changed
+63
-2
@@ -727,7 +727,7 @@ impl NativeAgent {
|
||||
fn handle_models_updated_event(
|
||||
&mut self,
|
||||
_registry: Entity<LanguageModelRegistry>,
|
||||
_event: &language_model::Event,
|
||||
event: &language_model::Event,
|
||||
cx: &mut Context<Self>,
|
||||
) {
|
||||
self.models.refresh_list(cx);
|
||||
@@ -744,7 +744,13 @@ impl NativeAgent {
|
||||
thread.set_model(model, cx);
|
||||
cx.notify();
|
||||
}
|
||||
thread.set_summarization_model(summarization_model.clone(), cx);
|
||||
if let Some(model) = summarization_model.clone() {
|
||||
if thread.summarization_model().is_none()
|
||||
|| matches!(event, language_model::Event::ThreadSummaryModelChanged)
|
||||
{
|
||||
thread.set_summarization_model(Some(model), cx);
|
||||
}
|
||||
}
|
||||
});
|
||||
}
|
||||
}
|
||||
@@ -2456,6 +2462,61 @@ mod internal_tests {
|
||||
});
|
||||
}
|
||||
|
||||
#[gpui::test]
|
||||
async fn test_summarization_model_survives_transient_registry_clearing(
|
||||
cx: &mut TestAppContext,
|
||||
) {
|
||||
init_test(cx);
|
||||
let fs = FakeFs::new(cx.executor());
|
||||
fs.insert_tree("/", json!({ "a": {} })).await;
|
||||
let project = Project::test(fs.clone(), [], cx).await;
|
||||
|
||||
let thread_store = cx.new(|cx| ThreadStore::new(cx));
|
||||
let agent =
|
||||
cx.update(|cx| NativeAgent::new(thread_store, Templates::new(), None, fs.clone(), cx));
|
||||
let connection = Rc::new(NativeAgentConnection(agent.clone()));
|
||||
|
||||
let acp_thread = cx
|
||||
.update(|cx| {
|
||||
connection.clone().new_session(
|
||||
project.clone(),
|
||||
PathList::new(&[Path::new("/a")]),
|
||||
cx,
|
||||
)
|
||||
})
|
||||
.await
|
||||
.unwrap();
|
||||
let session_id = acp_thread.read_with(cx, |thread, _| thread.session_id().clone());
|
||||
|
||||
let thread = agent.read_with(cx, |agent, _| {
|
||||
agent.sessions.get(&session_id).unwrap().thread.clone()
|
||||
});
|
||||
|
||||
thread.read_with(cx, |thread, _| {
|
||||
assert!(
|
||||
thread.summarization_model().is_some(),
|
||||
"session should have a summarization model from the test registry"
|
||||
);
|
||||
});
|
||||
|
||||
// Simulate what happens during a provider blip:
|
||||
// update_active_language_model_from_settings calls set_default_model(None)
|
||||
// when it can't resolve the model, clearing all fallbacks.
|
||||
cx.update(|cx| {
|
||||
LanguageModelRegistry::global(cx).update(cx, |registry, cx| {
|
||||
registry.set_default_model(None, cx);
|
||||
});
|
||||
});
|
||||
cx.run_until_parked();
|
||||
|
||||
thread.read_with(cx, |thread, _| {
|
||||
assert!(
|
||||
thread.summarization_model().is_some(),
|
||||
"summarization model should survive a transient default model clearing"
|
||||
);
|
||||
});
|
||||
}
|
||||
|
||||
#[gpui::test]
|
||||
async fn test_loaded_thread_preserves_thinking_enabled(cx: &mut TestAppContext) {
|
||||
init_test(cx);
|
||||
|
||||
Reference in new issue
Block a user