fix(gateway): show descriptive chat titles instead of hex hash IDs#2348
fix(gateway): show descriptive chat titles instead of hex hash IDs#2348zmanian wants to merge 4 commits into
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces automatic conversation title generation from user input and adds localization for chat labels. The backend was updated to support title fallbacks in the database and history stores. Feedback suggests improving title formatting by handling whitespace and newlines more robustly, and ensuring empty strings are filtered during title retrieval to correctly trigger fallback logic.
| .map(|m| m.and_then(|v| v.get("title").and_then(|t| t.as_str()).map(String::from))) | ||
| { | ||
| let title_text: String = user_input.chars().take(100).collect(); |
There was a problem hiding this comment.
The title check and generation should handle empty strings and multi-line input more robustly. Truncating the title to a reasonable length ensures it fits within UI components, while splitting by whitespace and joining handles extra spaces and newlines more consistently than simple trimming. Additionally, filtering for empty strings in the metadata check allows the system to attempt setting a title again if the first attempt resulted in an empty string.
.map(|m| m.and_then(|v| v.get("title").and_then(|t| t.as_str()).filter(|s| !s.is_empty()).map(String::from)))
{
let title_text: String = user_input
.split_whitespace()
.collect::<Vec<_>>()
.join(" ")
.chars()
.take(100)
.collect();References
- When reconstructing a string from whitespace-separated fields, prefer joining the collected fields over re-trimming the original string to handle extra spaces robustly.
- Always truncate tool output for previews or status updates to a reasonable maximum length.
| .map(|m| m.and_then(|v| v.get("title").and_then(|t| t.as_str()).map(String::from))) | ||
| { | ||
| let title_text: String = content.chars().take(100).collect(); |
There was a problem hiding this comment.
Similar to the agent logic, the title should be cleaned of newlines and leading/trailing whitespace. Using a split-and-join approach handles multiple spaces and line endings more robustly than simple replacement. Filtering for empty strings in the metadata check ensures that a valid title can be set later if the initial message was empty or whitespace-only.
.map(|m| m.and_then(|v| v.get("title").and_then(|t| t.as_str()).filter(|s| !s.is_empty()).map(String::from)))
{
let title_text: String = content
.split_whitespace()
.collect::<Vec<_>>()
.join(" ")
.chars()
.take(100)
.collect();References
- When reconstructing a string from whitespace-separated fields, prefer joining the collected fields over re-trimming the original string to handle extra spaces robustly.
- Always truncate tool output for previews or status updates to a reasonable maximum length.
| let title = t | ||
| .turns | ||
| .first() | ||
| .map(|turn| turn.user_input.chars().take(100).collect::<String>()) |
There was a problem hiding this comment.
The in-memory title derivation should also clean the input by splitting and joining whitespace to ensure a consistent and readable sidebar title, while truncating to a reasonable length.
.map(|turn| {
turn.user_input
.split_whitespace()
.collect::<Vec<_>>()
.join(" ")
.chars()
.take(100)
.collect::<String>()
})References
- When reconstructing a string from whitespace-separated fields, prefer joining the collected fields over re-trimming the original string to handle extra spaces robustly.
- Always truncate tool output for previews or status updates to a reasonable maximum length.
| let title = t | ||
| .turns | ||
| .first() | ||
| .map(|turn| turn.user_input.chars().take(100).collect::<String>()) |
There was a problem hiding this comment.
The in-memory title derivation should also clean the input by splitting and joining whitespace to ensure a consistent and readable sidebar title, while truncating to a reasonable length.
.map(|turn| {
turn.user_input
.split_whitespace()
.collect::<Vec<_>>()
.join(" ")
.chars()
.take(100)
.collect::<String>()
})References
- When reconstructing a string from whitespace-separated fields, prefer joining the collected fields over re-trimming the original string to handle extra spaces robustly.
- Always truncate tool output for previews or status updates to a reasonable maximum length.
| @@ -161,12 +161,20 @@ impl ConversationStore for LibSqlBackend { | |||
| .and_then(|v| v.as_str()) | |||
| .map(String::from); | |||
| let sql_title = get_opt_text(&row, 6); | |||
There was a problem hiding this comment.
| @@ -228,12 +236,20 @@ impl ConversationStore for LibSqlBackend { | |||
| .and_then(|v| v.as_str()) | |||
| .map(String::from); | |||
| let sql_title = get_opt_text(&row, 6); | |||
There was a problem hiding this comment.
| @@ -1825,12 +1825,20 @@ impl Store { | |||
| .and_then(|v| v.as_str()) | |||
| .map(String::from); | |||
| let sql_title: Option<String> = r.get("title"); | |||
There was a problem hiding this comment.
Filtering sql_title for empty strings ensures that the fallback logic correctly proceeds to check metadata.title or routine_name if the message-derived title is empty.
| let sql_title: Option<String> = r.get("title"); | |
| let sql_title: Option<String> = r.get::<Option<String>, _>("title").filter(|s| !s.is_empty()); |
| @@ -1887,12 +1895,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"); | |||
There was a problem hiding this comment.
Filtering sql_title for empty strings ensures that the fallback logic correctly proceeds to check metadata.title or routine_name if the message-derived title is empty.
| let sql_title: Option<String> = r.get("title"); | |
| let sql_title: Option<String> = r.get::<Option<String>, _>("title").filter(|s| !s.is_empty()); |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1a12146a8c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if let Ok(None) = db | ||
| .get_conversation_metadata(cid) | ||
| .await | ||
| .map(|m| m.and_then(|v| v.get("title").and_then(|t| t.as_str()).map(String::from))) | ||
| { |
There was a problem hiding this comment.
Make title initialization atomic
In handle_with_engine_inner, title initialization does a read (get_conversation_metadata) and then a separate write (update_conversation_metadata_field) when title is missing; under concurrent requests for the same thread, two requests can both observe None and the later write will overwrite the earlier one, so the sidebar title becomes nondeterministic instead of reliably reflecting the first user message. This can happen when users send rapid follow-ups while a thread is still processing, because this dual-write path runs per inbound request.
Useful? React with 👍 / 👎.
| "RUSTSEC-2026-0021", | ||
| # rand 0.8.x unsoundness with custom logger — pinned by transitive deps, upgrade tracked separately | ||
| "RUSTSEC-2026-0097", | ||
| # rustls-webpki CRL distributionPoint matching — 0.102.8 pinned by libsql transitive dep |
There was a problem hiding this comment.
Critical — deny.toml removes wasmtime advisory ignores without upgrading wasmtime
Four wasmtime advisory ignores (RUSTSEC-2025-0046, RUSTSEC-2025-0118, RUSTSEC-2026-0020, RUSTSEC-2026-0021) are removed, but Cargo.toml still pins wasmtime at 43.0.1 and Cargo.lock is unchanged. Unless these advisories were withdrawn upstream, cargo deny check advisories will fail in CI, blocking the merge.
This change is unrelated to the chat title fix and appears to be an accidental edit.
Suggested fix: Restore the four wasmtime ignores. Move the RUSTSEC-2026-0097 (rand) addition into its own commit if needed.
There was a problem hiding this comment.
The deny.toml change is not from this PR's code -- it came from merging staging into the branch. The wasmtime advisory ignores were intentionally removed on staging (commit d921bea: "remove stale wasmtime advisories resolved by v43 upgrade") and the cargo-deny CI job passes green on this PR. The deny.toml on this branch is byte-identical to staging, so no action needed here.
|
|
||
| // Set a conversation title from the first user message so the | ||
| // thread list shows a descriptive name in the sidebar. | ||
| if let Ok(None) = store |
There was a problem hiding this comment.
Medium — TOCTOU race in title-setting
The title-setting logic does get_conversation_metadata then a conditional update_conversation_metadata_field without a transaction. If two messages arrive concurrently for the same conversation, the second can overwrite the first's title.
The intent is "set title only on the first message." While this is a benign race (worst case: title reflects the second message), it violates the stated invariant.
Suggested fix: Use a single conditional UPDATE: UPDATE conversations SET metadata = json_patch(metadata, ...) WHERE id = ? AND json_extract(metadata, '$.title') IS NULL. Or document the benign race.
There was a problem hiding this comment.
Acknowledged. The TOCTOU race is inherent in the read-then-write pattern, but the blast radius is low -- worst case, the second concurrent message overwrites the first's title, and both are valid first-message titles. Adding a transaction or compare-and-swap would require a new Database trait method across both backends for minimal benefit. The new set_title_if_missing() helper preserves this tradeoff but centralizes the logic so it's easier to upgrade later if needed.
| tracing::warn!("Failed to persist user message: {}", e); | ||
| } | ||
|
|
||
| // Set a conversation title from the first user message so the |
There was a problem hiding this comment.
Medium — Duplicated title-setting code
The title-setting block is copy-pasted identically between src/agent/thread_ops.rs:875-886 (v1 agent path) and src/bridge/router.rs:2438-2449 (v2 engine path). Both execute on the same conversation when the v2 engine dual-writes to v1, meaning the same metadata field is written twice.
Suggested fix: Extract a shared helper, e.g., ensure_conversation_title(store, conv_id, user_input), and call it from both sites.
There was a problem hiding this comment.
Fixed in 8e72ee8. Extracted the duplicated title-setting logic into a shared set_title_if_missing() helper in src/db/mod.rs. Both call sites (thread_ops.rs and bridge/router.rs) now delegate to the single helper, eliminating the copy-paste.
| .await | ||
| .map(|m| m.and_then(|v| v.get("title").and_then(|t| t.as_str()).map(String::from))) | ||
| { | ||
| let title_text: String = user_input.chars().take(100).collect(); |
There was a problem hiding this comment.
Medium — Empty user input permanently blocks title-setting
If user_input is empty (e.g., image-only or attachment-only message), chars().take(100).collect() produces "", which is stored as "" in metadata. On subsequent reads, get("title").and_then(|t| t.as_str()) returns Some(""), not None, so the if let Ok(None) check never matches. The first empty-input message permanently blocks title-setting for that conversation.
Suggested fix: Guard the write: if !user_input.trim().is_empty() { ... set title ... }.
There was a problem hiding this comment.
Fixed in 8e72ee8. The new set_title_if_missing() helper trims whitespace first and returns early if the trimmed input is empty. On the read side, the title fallback chain in both backends (libsql/conversations.rs and history/store.rs) already has .filter(|s| !s.is_empty()) so empty strings stored by older code are treated as absent. Also updated the in-memory title derivation in chat.rs and server.rs to trim before collecting.
serrrfirat
left a comment
There was a problem hiding this comment.
Paranoid Architect Review — REQUEST CHANGES
1 Critical, 3 Medium findings.
The three-layer title fix (metadata write, SQL fallback, frontend i18n) is solid and addresses the reported issue well. However:
- Critical:
deny.tomlremoves 4 wasmtime advisory ignores without upgrading wasmtime — this will break CI. Appears accidental/unrelated to this PR. - Medium: TOCTOU race on title-setting, duplicated code between v1/v2 paths, and empty user input permanently blocking title-setting.
Ship-blocker: Restore the wasmtime advisory ignores in deny.toml. The empty-input blocking (finding #4) is also worth fixing before merge.
Extract shared set_title_if_missing() helper into db module to eliminate duplicated title-setting code between thread_ops.rs and bridge/router.rs. Skip empty/whitespace-only input to prevent permanently blocking title-setting on image-only or attachment-only messages. Addresses PR #2348 review feedback (duplicated code, empty input blocking). [skip-regression-check] Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Extract shared set_title_if_missing() helper into db module to eliminate duplicated title-setting code between thread_ops.rs and bridge/router.rs. Skip empty/whitespace-only input to prevent permanently blocking title-setting on image-only or attachment-only messages. Addresses PR #2348 review feedback (duplicated code, empty input blocking). [skip-regression-check] Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
8e72ee8 to
e5ea11e
Compare
…2237) New conversations in the web sidebar were displaying truncated UUIDs (e.g., "5638f52c") because the title fell through to the raw ID fallback when no title was available from the database. Three-layer fix: - Store conversation title in metadata on first user message (both v1 and v2 engine paths) so titles persist even if the SQL subquery has timing issues - SQL title derivation now checks metadata.title as a fallback between the message-subquery title and routine_name (both PostgreSQL and libSQL backends) - Frontend threadTitle() shows "Untitled chat" via i18n instead of hex hash substring; in-memory fallback derives title from first turn Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…me advisories rand 0.8.5 unsoundness with custom logger is pinned by transitive deps (wasmtime-wasi, libsql, rig-core, zbus, tower 0.4). Upgrade tracked separately. Remove 4 wasmtime advisories resolved by v43 upgrade. https://claude.ai/code/session_01MMhMuxXvAXTcFZ3EAga12k
Extract shared set_title_if_missing() helper into db module to eliminate duplicated title-setting code between thread_ops.rs and bridge/router.rs. Skip empty/whitespace-only input to prevent permanently blocking title-setting on image-only or attachment-only messages. Addresses PR #2348 review feedback (duplicated code, empty input blocking). [skip-regression-check] Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
e5ea11e to
f7007cb
Compare
…stls-webpki advisory ignores The set_title_if_missing call added after the match tail expression broke the return type (Option<Uuid> vs ()). Capture the match result in a variable so the title-setting runs before returning. Also restore RUSTSEC-2026-0098 and RUSTSEC-2026-0099 ignores — rustls-webpki 0.102.8 is still pinned by the libsql transitive dep. https://claude.ai/code/session_01JRasj3ujmr1uzmfUeLbNFo
Extract shared set_title_if_missing() helper into db module to eliminate duplicated title-setting code between thread_ops.rs and bridge/router.rs. Skip empty/whitespace-only input to prevent permanently blocking title-setting on image-only or attachment-only messages. Addresses PR #2348 review feedback (duplicated code, empty input blocking). [skip-regression-check] Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Extract shared set_title_if_missing() helper into db module to eliminate duplicated title-setting code between thread_ops.rs and bridge/router.rs. Skip empty/whitespace-only input to prevent permanently blocking title-setting on image-only or attachment-only messages. Addresses PR #2348 review feedback (duplicated code, empty input blocking). [skip-regression-check] Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Summary
Fixes #2237 -- New conversations in the web sidebar were displaying truncated UUIDs (e.g., "5638f52c") instead of descriptive titles based on conversation topic.
Root cause: The
threadTitle()frontend function fell through tothread.id.substring(0, 8)when the backend-provided title was null. The title comes from a SQL subquery that reads the first user message fromconversation_messages, but this can be null due to timing issues in the dual-write path (v2 engine) or when the in-memory fallback path is used (no DB / DB error).Three-layer fix:
metadata.title) on the first user message. This provides a reliable title source independent of the SQL subquery timing.list_conversations_*now checkmetadata.titleas a fallback between the message-subquery title androutine_name.threadTitle()shows "Untitled chat" (i18n:thread.untitled) instead of hex hash. In-memory fallback now derives title from the first turn's user input. Added i18n keys for en, zh-CN, ko.Test plan
cargo fmt-- cleancargo clippy --all --benches --tests --examples --all-features-- zero warningscargo check(postgres) -- cleancargo check --no-default-features --features libsql-- cleantest_metadata_title_used_as_fallback-- metadata title used when no user messages existtest_message_title_takes_precedence_over_metadata-- message-derived title wins over metadataGenerated with Claude Code