-
Notifications
You must be signed in to change notification settings - Fork 1.4k
fix(gateway): show descriptive chat titles instead of hex hash IDs #2348
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
36e2196
2536616
f7007cb
09ff9a5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -171,12 +171,20 @@ impl ConversationStore for LibSqlBackend { | |
| .and_then(|v| v.as_str()) | ||
| .map(String::from); | ||
| let sql_title = get_opt_text(&row, 6); | ||
| let title = sql_title.or_else(|| { | ||
| metadata | ||
| .get("routine_name") | ||
| .and_then(|v| v.as_str()) | ||
| .map(String::from) | ||
| }); | ||
| let title = sql_title | ||
| .or_else(|| { | ||
| metadata | ||
| .get("title") | ||
| .and_then(|v| v.as_str()) | ||
| .filter(|s| !s.is_empty()) | ||
| .map(String::from) | ||
| }) | ||
| .or_else(|| { | ||
| metadata | ||
| .get("routine_name") | ||
| .and_then(|v| v.as_str()) | ||
| .map(String::from) | ||
| }); | ||
| results.push(ConversationSummary { | ||
| id: row | ||
| .get::<String>(0) | ||
|
|
@@ -250,12 +258,20 @@ impl ConversationStore for LibSqlBackend { | |
| .and_then(|v| v.as_str()) | ||
| .map(String::from); | ||
| let sql_title = get_opt_text(&row, 6); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
| let title = sql_title.or_else(|| { | ||
| metadata | ||
| .get("routine_name") | ||
| .and_then(|v| v.as_str()) | ||
| .map(String::from) | ||
| }); | ||
| let title = sql_title | ||
| .or_else(|| { | ||
| metadata | ||
| .get("title") | ||
| .and_then(|v| v.as_str()) | ||
| .filter(|s| !s.is_empty()) | ||
| .map(String::from) | ||
| }) | ||
| .or_else(|| { | ||
| metadata | ||
| .get("routine_name") | ||
| .and_then(|v| v.as_str()) | ||
| .map(String::from) | ||
| }); | ||
| results.push(ConversationSummary { | ||
| id: row | ||
| .get::<String>(0) | ||
|
|
@@ -1009,4 +1025,85 @@ mod tests { | |
| "assistant thread lookup should backfill a legacy NULL source_channel" | ||
| ); | ||
| } | ||
|
|
||
| /// Regression test for #2237: conversations with a metadata title should | ||
| /// use it as a fallback when the message-derived title is NULL. | ||
| #[tokio::test] | ||
| async fn test_metadata_title_used_as_fallback() { | ||
| let dir = tempfile::tempdir().unwrap(); | ||
| let db_path = dir.path().join("test_metadata_title.db"); | ||
| let backend = LibSqlBackend::new_local(&db_path).await.unwrap(); | ||
| backend.run_migrations().await.unwrap(); | ||
|
|
||
| let conv_id = Uuid::new_v4(); | ||
| let user_id = "user-title-test"; | ||
|
|
||
| // Create a conversation with no messages | ||
| backend | ||
| .ensure_conversation(conv_id, "gateway", user_id, None, Some("gateway")) | ||
| .await | ||
| .unwrap(); | ||
|
|
||
| // Set a metadata title (simulating what persist_user_message does) | ||
| let title_val = serde_json::json!("What is the weather today?"); | ||
| backend | ||
| .update_conversation_metadata_field(conv_id, "title", &title_val) | ||
| .await | ||
| .unwrap(); | ||
|
|
||
| // List conversations -- title should come from metadata even without messages | ||
| let convs = backend | ||
| .list_conversations_all_channels(user_id, 50) | ||
| .await | ||
| .unwrap(); | ||
|
|
||
| let conv = convs.iter().find(|c| c.id == conv_id).unwrap(); | ||
| assert_eq!( | ||
| conv.title.as_deref(), | ||
| Some("What is the weather today?"), | ||
| "Conversation title should fall back to metadata title when no user messages exist" | ||
| ); | ||
| } | ||
|
|
||
| /// Regression test for #2237: message-derived title takes precedence over metadata. | ||
| #[tokio::test] | ||
| async fn test_message_title_takes_precedence_over_metadata() { | ||
| let dir = tempfile::tempdir().unwrap(); | ||
| let db_path = dir.path().join("test_message_title_precedence.db"); | ||
| let backend = LibSqlBackend::new_local(&db_path).await.unwrap(); | ||
| backend.run_migrations().await.unwrap(); | ||
|
|
||
| let conv_id = Uuid::new_v4(); | ||
| let user_id = "user-title-precedence"; | ||
|
|
||
| backend | ||
| .ensure_conversation(conv_id, "gateway", user_id, None, Some("gateway")) | ||
| .await | ||
| .unwrap(); | ||
|
|
||
| // Set metadata title | ||
| let title_val = serde_json::json!("metadata title"); | ||
| backend | ||
| .update_conversation_metadata_field(conv_id, "title", &title_val) | ||
| .await | ||
| .unwrap(); | ||
|
|
||
| // Add a user message | ||
| backend | ||
| .add_conversation_message(conv_id, "user", "actual user message") | ||
| .await | ||
| .unwrap(); | ||
|
|
||
| let convs = backend | ||
| .list_conversations_all_channels(user_id, 50) | ||
| .await | ||
| .unwrap(); | ||
|
|
||
| let conv = convs.iter().find(|c| c.id == conv_id).unwrap(); | ||
| assert_eq!( | ||
| conv.title.as_deref(), | ||
| Some("actual user message"), | ||
| "Message-derived title should take precedence over metadata title" | ||
| ); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -1839,12 +1839,20 @@ impl Store { | |||||
| .and_then(|v| v.as_str()) | ||||||
| .map(String::from); | ||||||
| let sql_title: Option<String> = r.get("title"); | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Filtering
Suggested change
|
||||||
| let title = sql_title.or_else(|| { | ||||||
| metadata | ||||||
| .get("routine_name") | ||||||
| .and_then(|v| v.as_str()) | ||||||
| .map(String::from) | ||||||
| }); | ||||||
| let title = sql_title | ||||||
| .or_else(|| { | ||||||
| metadata | ||||||
| .get("title") | ||||||
| .and_then(|v| v.as_str()) | ||||||
| .filter(|s| !s.is_empty()) | ||||||
| .map(String::from) | ||||||
| }) | ||||||
| .or_else(|| { | ||||||
| metadata | ||||||
| .get("routine_name") | ||||||
| .and_then(|v| v.as_str()) | ||||||
| .map(String::from) | ||||||
| }); | ||||||
| ConversationSummary { | ||||||
| id: r.get("id"), | ||||||
| title, | ||||||
|
|
@@ -1913,12 +1921,20 @@ impl Store { | |||||
| // For routine/heartbeat threads, derive title from metadata | ||||||
| // since they may have no user messages. | ||||||
| let sql_title: Option<String> = r.get("title"); | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Filtering
Suggested change
|
||||||
| let title = sql_title.or_else(|| { | ||||||
| metadata | ||||||
| .get("routine_name") | ||||||
| .and_then(|v| v.as_str()) | ||||||
| .map(String::from) | ||||||
| }); | ||||||
| let title = sql_title | ||||||
| .or_else(|| { | ||||||
| metadata | ||||||
| .get("title") | ||||||
| .and_then(|v| v.as_str()) | ||||||
| .filter(|s| !s.is_empty()) | ||||||
| .map(String::from) | ||||||
| }) | ||||||
| .or_else(|| { | ||||||
| metadata | ||||||
| .get("routine_name") | ||||||
| .and_then(|v| v.as_str()) | ||||||
| .map(String::from) | ||||||
| }); | ||||||
| ConversationSummary { | ||||||
| id: r.get("id"), | ||||||
| title, | ||||||
|
|
||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Filtering
sql_titlefor empty strings ensures that the fallback logic correctly proceeds to checkmetadata.titleorroutine_nameif the message-derived title is empty.