fix(gateway): show descriptive chat titles instead of hex hash IDs (#2237)#2700
fix(gateway): show descriptive chat titles instead of hex hash IDs (#2237)#2700
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f07b49c51b
ℹ️ 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".
There was a problem hiding this comment.
Code Review
This pull request implements automatic conversation title generation and improves i18n support for chat threads. It introduces a mechanism to set a title from the first user message if one is missing and updates the database logic to prioritize these titles. Feedback focuses on ensuring consistent whitespace normalization across all title derivation paths and optimizing error handling when conversation metadata is missing.
Code ReviewOverviewThree-layer fix so new conversations show a real title in the web sidebar instead of
CI is green across fmt/clippy/deny/replay; only en/ko/zh-CN locales exist, so translations are complete. Strengths
Issues & SuggestionsCorrectness
Performance
Test coverage
Security
Style / conventions
Risk assessmentLow risk. Fully additive: existing RecommendationApprove with minor nits. Worth addressing the |
…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>
…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
…g conversation Addresses review feedback on PR #2700: - Return early from set_title_if_missing when conversation record doesn't exist (Ok(None)) to avoid a failing metadata update attempt - Normalize whitespace in title_text (collapse multi-space/newlines) - Filter blank sql_title values before metadata fallback in both libSQL and PostgreSQL paths so image-only/empty first turns properly fall through to metadata.title https://claude.ai/code/session_01GHzXcZjSjxHEeTDDHG4fQK
f07b49c to
716b878
Compare
…log metadata errors - Move set_title_if_missing call inside Ok arm so failed message persists don't produce ghost titles (ilblackdragon review) - Log metadata read errors at debug level instead of silently swallowing [skip-regression-check] https://claude.ai/code/session_01BsM2KUzdZNBeoLjk7LymZS
|
Addressed @ilblackdragon's review nits in de2876b:
Clippy clean, zero warnings. Generated by Claude Code |
serrrfirat
left a comment
There was a problem hiding this comment.
Re-checked this against current staging. The underlying sidebar-title bug is still real, but I found a few remaining correctness gaps before merge; details inline.
…, expand tests Three review issues on #2700: 1. Title source (router.rs + thread_ops.rs). Titles were derived from `effective_content`, which is the attachment-augmented payload produced by `augment_with_attachments` — it can include a synthesized `<attachments>` block or extracted OCR/pdf text. On an image- or attachment-only first turn this meant the sidebar title was the synthesized attachment text rather than user input, which defeats the point of skipping empty turns. Both call sites now pass the raw user `content` to `set_title_if_missing` while the conversation row body continues to store the augmented payload. `persist_user_message` takes a new `title_source` argument to keep the two distinct on the v1 path. 2. Ghost title on failed persist (router.rs). The v2 bridge dual-write discarded the `add_conversation_message` result with `let _`, so `set_title_if_missing` ran even when the first user row never landed — reproducing the ghost-title edge case thread_ops.rs already guards against. The result is now inspected and the title write is gated on `Ok`. 3. Test coverage (store.rs + thread_ops.rs). The libSQL regression tests don't pin the PG path or the caller wiring. Adds: - PG mirrors of `test_metadata_title_used_as_fallback` and `test_message_title_takes_precedence_over_metadata` behind `feature = "postgres"` + `#[ignore]` (integration tier). - A caller-level regression in `thread_ops` that drives `Agent::persist_user_message` against a real LibSqlBackend and pins all three invariants: attachment-augmented payload with real text uses the raw text; attachment-only first turn leaves the title unset; plain text still seeds correctly. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
I'm not sure if this is still an issue? |
…tle write atomic Addresses three unresolved review comments on #2700: 1. In-memory threads fallback (src/channels/web/features/chat/mod.rs). The no-DB path in `chat_threads_handler` still derived its sidebar title from `turn.user_input`, which is the attachment-augmented payload stamped by `process_user_input`. Adds a new `raw_user_input: Option<String>` field to `Turn`, populated when augmentation actually changes the text, and a shared `title_from_in_memory_turn` helper that prefers the raw text. Plain turns leave the field `None` and fall through to `user_input` cleanly. 2. Engine-v2 path (src/bridge/router.rs + crates/ironclaw_engine/). `ConversationManager::handle_user_message` now takes an explicit `raw_content_for_title: Option<&str>` parameter. The LLM-facing `content` stays as the attachment-augmented payload; the raw text is stored separately on the `ConversationEntry.metadata` as `title_source` (via new `ConversationEntry::user_with_title_source`) so downstream title consumers see the user-typed text, not the synthesized `<attachments>` block. The router passes `content` (raw) as the title source alongside `effective_content`. 3. Atomic title write (src/db/mod.rs + libSQL + PG). Replaces the check-then-update pair in `set_title_if_missing` with a conditional single-statement UPDATE on both backends. libSQL uses `json_extract(metadata, '$.title') IS NULL OR = ''`; PG uses `NOT (metadata ? 'title') OR metadata->>'title' = ''`. Adds `ConversationStore::set_conversation_title_if_empty` returning whether the write landed, so concurrent first-turn callers cannot race to overwrite each other. Tests: - `test_set_title_if_empty_is_atomic_under_concurrency` (libSQL) races two `tokio::join!`'d calls and asserts exactly one wins. - `test_set_title_if_empty_writes_when_unset` covers NULL metadata and empty-string title rows. - `test_set_title_if_empty_is_atomic_pg` mirrors the race on PG (integration tier, `#[ignore]`). - `handle_user_message_records_raw_title_source` and `handle_user_message_plain_text_has_no_title_source_metadata` pin the engine-v2 plumbing. - `test_title_from_in_memory_turn_*` (3 cases) cover the fallback helper across augmented, plain, and empty-raw turns. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
# Conflicts: # crates/ironclaw_engine/src/runtime/conversation.rs
|
Update to get this unstuck:
Local verification:
The existing re-review request for @serrrfirat is still active. |
Replaces #2348 — recreated on fresh staging after gateway and web handler refactors (#2683, #2599).
Summary
Three-layer fix for new conversations displaying truncated UUIDs in the web sidebar:
set_title_if_missing()helper insrc/db/mod.rs. Empty/whitespace-only input is skipped so image-only messages don't permanently block title setting.metadata.titleas a fallback between the message-subquery title androutine_name.threadTitle()(ported tojs/core/history.jspost-refactor(gateway): split monolithic style.css and app.js into per-surface modules #2683) shows localized "Untitled chat" instead of hex hash substring. In-memory fallback derives title from first turn's input with whitespace normalization (addresses review feedback about multi-line input).Files changed
src/channels/web/features/chat/mod.rs— in-memory title derivation (ported from handlers/chat.rs post-Epic: Enforce gateway feature boundaries, crate guardrails, and crate-owned E2E #2599)src/agent/thread_ops.rs,src/bridge/router.rs— both call sharedset_title_if_missing()src/db/mod.rs—set_title_if_missing()helpersrc/db/libsql/conversations.rs,src/history/store.rs— metadata.title fallback in SQL title derivationcrates/ironclaw_gateway/static/js/core/history.js—threadTitle()i18ncrates/ironclaw_gateway/static/i18n/{en,ko,zh-CN}.js— new keysNotes
Test plan
cargo check -p ironclawcleanCloses #2237