From 109bda35ac480ea2329690f47791469bcc701696 Mon Sep 17 00:00:00 2001 From: Lifei Zhou Date: Thu, 18 Jun 2026 08:18:28 +1000 Subject: [PATCH] refactor: make session.name the source of truth for displayed session titles (#9841) --- crates/goose/src/acp/response_builder.rs | 5 +- crates/goose/src/session/session_manager.rs | 254 +++++++++++++++++--- ui/desktop/src/__tests__/sessions.test.ts | 48 +--- ui/desktop/src/sessions.ts | 18 +- 4 files changed, 237 insertions(+), 88 deletions(-) diff --git a/crates/goose/src/acp/response_builder.rs b/crates/goose/src/acp/response_builder.rs index c16d7463eb..0201c439e4 100644 --- a/crates/goose/src/acp/response_builder.rs +++ b/crates/goose/src/acp/response_builder.rs @@ -75,12 +75,11 @@ pub(super) fn session_meta(session: &Session) -> serde_json::Map SessionInfo { let meta = session_meta(&session); - let title = session.display_title(); let mut info = SessionInfo::new(SessionId::new(session.id), session.working_dir) .updated_at(session.updated_at.to_rfc3339()) .meta(meta); - if let Some(title) = title { - info = info.title(title); + if !session.name.is_empty() { + info = info.title(session.name); } info } diff --git a/crates/goose/src/session/session_manager.rs b/crates/goose/src/session/session_manager.rs index a5910bd455..f81da3c9e0 100644 --- a/crates/goose/src/session/session_manager.rs +++ b/crates/goose/src/session/session_manager.rs @@ -90,21 +90,6 @@ pub struct Session { pub last_message_snippet: Option, } -impl Session { - pub fn display_title(&self) -> Option { - if !self.user_set_name && self.session_type != SessionType::Scheduled { - if let Some(recipe) = &self.recipe { - return Some(recipe.title.clone()); - } - } - if self.name.is_empty() { - None - } else { - Some(self.name.clone()) - } - } -} - pub struct SessionUpdateBuilder<'a> { session_manager: &'a SessionManager, session_id: String, @@ -468,6 +453,26 @@ impl SessionManager { .await } + async fn system_generated_name_update( + &self, + id: &str, + name: String, + ) -> Result { + self.update(id) + .system_generated_name(name.clone()) + .apply() + .await?; + + let session = self.get_session(id, false).await?; + Ok(SessionNameUpdate { + session_id: id.to_string(), + name, + updated_at: session.updated_at, + message_count: session.message_count, + user_set_name: session.user_set_name, + }) + } + pub async fn maybe_update_name( &self, id: &str, @@ -479,6 +484,19 @@ impl SessionManager { return Ok(None); } + if session.session_type == SessionType::Scheduled { + return Ok(None); + } + + if let Some(recipe) = &session.recipe { + let name = recipe.title.trim().to_string(); + if name.is_empty() || session.name == name { + return Ok(None); + } + + return Ok(Some(self.system_generated_name_update(id, name).await?)); + } + let conversation = session .conversation .ok_or_else(|| anyhow::anyhow!("No messages found"))?; @@ -491,19 +509,7 @@ impl SessionManager { if user_message_count <= MSG_COUNT_FOR_SESSION_NAME_GENERATION { let name = provider.generate_session_name(id, &conversation).await?; - self.update(id) - .system_generated_name(name.clone()) - .apply() - .await?; - - let session = self.get_session(id, false).await?; - return Ok(Some(SessionNameUpdate { - session_id: id.to_string(), - name, - updated_at: session.updated_at, - message_count: session.message_count, - user_set_name: session.user_set_name, - })); + return Ok(Some(self.system_generated_name_update(id, name).await?)); } Ok(None) } @@ -2035,10 +2041,61 @@ fn merge_tool_meta( mod tests { use super::*; use crate::conversation::message::{Message, MessageContent}; + use crate::providers::base::MessageStream; use tempfile::TempDir; use test_case::test_case; const NUM_CONCURRENT_SESSIONS: i32 = 10; + const GENERATED_SESSION_NAME: &str = "Generated session name"; + + struct NamingTestProvider { + model_config: ModelConfig, + } + + #[async_trait::async_trait] + impl Provider for NamingTestProvider { + fn get_name(&self) -> &str { + "naming-test" + } + + async fn stream( + &self, + _model_config: &ModelConfig, + _session_id: &str, + _system: &str, + _messages: &[Message], + _tools: &[rmcp::model::Tool], + ) -> std::result::Result { + unimplemented!("session naming tests override generate_session_name") + } + + fn get_model_config(&self) -> ModelConfig { + self.model_config.clone() + } + + async fn generate_session_name( + &self, + _session_id: &str, + _messages: &Conversation, + ) -> std::result::Result { + Ok(GENERATED_SESSION_NAME.to_string()) + } + } + + fn naming_test_provider() -> Arc { + Arc::new(NamingTestProvider { + model_config: ModelConfig::new("test-model").unwrap(), + }) + } + + fn test_recipe(title: &str) -> Recipe { + Recipe::builder() + .title(title) + .description("Recipe description") + .instructions("Follow the recipe") + .build() + .unwrap() + } async fn create_session_for_list( sm: &SessionManager, @@ -2115,6 +2172,147 @@ mod tests { .unwrap(); } + async fn add_user_message(sm: &SessionManager, session_id: &str) { + sm.add_message(session_id, &Message::user().with_text("hello world")) + .await + .unwrap(); + } + + #[tokio::test] + async fn test_maybe_update_name_updates_eligible_session() { + let temp_dir = TempDir::new().unwrap(); + let sm = SessionManager::new(temp_dir.path().to_path_buf()); + + let session = sm + .create_session( + temp_dir.path().to_path_buf(), + "New Chat".to_string(), + SessionType::User, + GooseMode::default(), + ) + .await + .unwrap(); + + add_user_message(&sm, &session.id).await; + + let update = sm + .maybe_update_name(&session.id, naming_test_provider()) + .await + .unwrap(); + assert_eq!( + update.as_ref().map(|update| update.name.as_str()), + Some(GENERATED_SESSION_NAME) + ); + + let reloaded = sm.get_session(&session.id, false).await.unwrap(); + assert_eq!(reloaded.name, GENERATED_SESSION_NAME); + assert!(!reloaded.user_set_name); + } + + #[tokio::test] + async fn test_maybe_update_name_preserves_user_renamed_session() { + let temp_dir = TempDir::new().unwrap(); + let sm = SessionManager::new(temp_dir.path().to_path_buf()); + + let session = sm + .create_session( + temp_dir.path().to_path_buf(), + "New Chat".to_string(), + SessionType::User, + GooseMode::default(), + ) + .await + .unwrap(); + + sm.update(&session.id) + .user_provided_name("Manual title".to_string()) + .apply() + .await + .unwrap(); + add_user_message(&sm, &session.id).await; + + let update = sm + .maybe_update_name(&session.id, naming_test_provider()) + .await + .unwrap(); + assert!(update.is_none()); + + let reloaded = sm.get_session(&session.id, false).await.unwrap(); + assert_eq!(reloaded.name, "Manual title"); + assert!(reloaded.user_set_name); + } + + #[tokio::test] + async fn test_maybe_update_name_uses_recipe_title_for_recipe_session() { + let temp_dir = TempDir::new().unwrap(); + let sm = SessionManager::new(temp_dir.path().to_path_buf()); + + let session = sm + .create_session( + temp_dir.path().to_path_buf(), + "New Chat".to_string(), + SessionType::User, + GooseMode::default(), + ) + .await + .unwrap(); + + sm.update(&session.id) + .recipe(Some(test_recipe("Recipe title"))) + .apply() + .await + .unwrap(); + add_user_message(&sm, &session.id).await; + + let update = sm + .maybe_update_name(&session.id, naming_test_provider()) + .await + .unwrap(); + assert_eq!( + update.as_ref().map(|update| update.name.as_str()), + Some("Recipe title") + ); + + let reloaded = sm.get_session(&session.id, false).await.unwrap(); + assert_eq!(reloaded.name, "Recipe title"); + assert!(!reloaded.user_set_name); + + let update = sm + .maybe_update_name(&session.id, naming_test_provider()) + .await + .unwrap(); + assert!(update.is_none()); + } + + #[tokio::test] + async fn test_maybe_update_name_preserves_scheduled_session() { + let temp_dir = TempDir::new().unwrap(); + let sm = SessionManager::new(temp_dir.path().to_path_buf()); + let original_name = "Scheduled job: test-job"; + + let session = sm + .create_session( + temp_dir.path().to_path_buf(), + original_name.to_string(), + SessionType::Scheduled, + GooseMode::default(), + ) + .await + .unwrap(); + + add_user_message(&sm, &session.id).await; + + let update = sm + .maybe_update_name(&session.id, naming_test_provider()) + .await + .unwrap(); + assert!(update.is_none()); + + let reloaded = sm.get_session(&session.id, false).await.unwrap(); + assert_eq!(reloaded.name, original_name); + assert!(!reloaded.user_set_name); + } + async fn create_search_session( sm: &SessionManager, name: &str, diff --git a/ui/desktop/src/__tests__/sessions.test.ts b/ui/desktop/src/__tests__/sessions.test.ts index e865e20190..e76650282b 100644 --- a/ui/desktop/src/__tests__/sessions.test.ts +++ b/ui/desktop/src/__tests__/sessions.test.ts @@ -1,5 +1,5 @@ import { describe, it, expect } from 'vitest'; -import { getSessionDisplayName, shouldShowNewChatTitle } from '../sessions'; +import { getSessionDisplayName } from '../sessions'; import { prependUnique } from '../hooks/useNavigationSessions'; import type { Session } from '../api'; import type { SessionListItem } from '../acp/sessions'; @@ -30,51 +30,19 @@ function makeListItem(overrides: Partial = {}): SessionListItem }; } -describe('shouldShowNewChatTitle', () => { - it('returns true for an empty session without a user-set name', () => { - const session = makeSession({ message_count: 0, user_set_name: false }); - expect(shouldShowNewChatTitle(session)).toBe(true); - }); - - it('returns false when the session has messages', () => { - const session = makeSession({ message_count: 3, user_set_name: false }); - expect(shouldShowNewChatTitle(session)).toBe(false); - }); - - it('returns false when the user has set a custom name', () => { - const session = makeSession({ message_count: 0, user_set_name: true }); - expect(shouldShowNewChatTitle(session)).toBe(false); - }); - - it('returns false when the session has a recipe', () => { +describe('getSessionDisplayName', () => { + it('returns the session name', () => { const session = makeSession({ - message_count: 0, - user_set_name: false, - recipe: { title: 'Recipe', steps: [] } as unknown as Session['recipe'], + name: 'My Chat', }); - expect(shouldShowNewChatTitle(session)).toBe(false); - }); -}); - -describe('getSessionDisplayName (fix for #8865)', () => { - it('returns the user-set name for a recipe session that has been renamed', () => { - const session = makeSession({ - name: 'My Renamed Chat', - user_set_name: true, - message_count: 2, - recipe: { title: 'Some Recipe' } as unknown as Session['recipe'], - }); - expect(getSessionDisplayName(session)).toBe('My Renamed Chat'); + expect(getSessionDisplayName(session)).toBe('My Chat'); }); - it('falls back to the recipe title when the user has not renamed', () => { + it('falls back to the default title when the session name is empty', () => { const session = makeSession({ - name: 'auto-generated', - user_set_name: false, - message_count: 2, - recipe: { title: 'Some Recipe' } as unknown as Session['recipe'], + name: '', }); - expect(getSessionDisplayName(session)).toBe('Some Recipe'); + expect(getSessionDisplayName(session)).toBe('New Chat'); }); }); diff --git a/ui/desktop/src/sessions.ts b/ui/desktop/src/sessions.ts index 33cfd1bf6b..06d10217a2 100644 --- a/ui/desktop/src/sessions.ts +++ b/ui/desktop/src/sessions.ts @@ -10,24 +10,8 @@ import type { FixedExtensionEntry } from './components/ConfigContext'; import { AppEvents } from './constants/events'; import { decodeRecipe, Recipe } from './recipe'; -export function shouldShowNewChatTitle(session: Session): boolean { - if (session.recipe) { - return false; - } - return !session.user_set_name && session.message_count === 0; -} - export function getSessionDisplayName(session: Session): string { - if (session.user_set_name) { - return session.name; - } - if (session.recipe?.title) { - return session.recipe.title; - } - if (shouldShowNewChatTitle(session)) { - return DEFAULT_CHAT_TITLE; - } - return session.name; + return session.name || DEFAULT_CHAT_TITLE; } export function resumeSession(session: Session, setView: setViewType) {