fix: flatten rich message cards into text for webchat frontend#512
fix: flatten rich message cards into text for webchat frontend#512TheDarkSkyXD wants to merge 2 commits intospacedriveapp:mainfrom
Conversation
The webchat frontend doesn't support rich embeds (cards), so card content was being lost when messages used the RichMessage variant. This flattens card titles, descriptions, and fields into plain text across SSE forwarding, broadcast, conversation logging, and cancelled-history extraction. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
WalkthroughAdds handling of Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/tools/reply.rs (1)
476-490: Centralizetext + cardsmerge policy to avoid drift.This merge logic is now duplicated across multiple paths (
src/tools/reply.rs,src/main.rs,src/messaging/webchat.rs, and cancelled-history extraction). Consider one shared helper onOutboundResponsefor consistent behavior everywhere.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tools/reply.rs` around lines 476 - 490, Create a single helper on OutboundResponse (e.g., a method named flattened_text_or_merge or merge_text_and_cards) that encapsulates the current logic: call OutboundResponse::text_from_cards(cards) when self is RichMessage, then return converted_content if card text is empty, card text if converted_content is empty/whitespace, or the two joined with "\n\n" otherwise; use that method in reply.rs (replace the inline block around OutboundResponse::RichMessage usage), and replace the duplicated logic in main.rs, messaging/webchat.rs and the cancelled-history extraction with calls to this new OutboundResponse helper so the merge policy is centralized and consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/agent/channel_history.rs`:
- Around line 172-193: The current extraction is gated on finding "content" as a
string; instead, always attempt to flatten cards from
tool_call.function.arguments.get("cards") via
serde_json::from_value::<Vec<crate::Card>>() and
crate::OutboundResponse::text_from_cards(&cards), then separately check for an
optional content string from
tool_call.function.arguments.get("content").as_str(); finally, return the
combined result: if both text and card_text are non-empty return
format!("{}\n\n{}", text, card_text), if only text return text, if only
card_text return card_text, otherwise None—update the logic around
tool_call.function.arguments, content, cards, and
crate::OutboundResponse::text_from_cards to implement this.
---
Nitpick comments:
In `@src/tools/reply.rs`:
- Around line 476-490: Create a single helper on OutboundResponse (e.g., a
method named flattened_text_or_merge or merge_text_and_cards) that encapsulates
the current logic: call OutboundResponse::text_from_cards(cards) when self is
RichMessage, then return converted_content if card text is empty, card text if
converted_content is empty/whitespace, or the two joined with "\n\n" otherwise;
use that method in reply.rs (replace the inline block around
OutboundResponse::RichMessage usage), and replace the duplicated logic in
main.rs, messaging/webchat.rs and the cancelled-history extraction with calls to
this new OutboundResponse helper so the merge policy is centralized and
consistent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7d7a2827-b5eb-4fbc-8e84-cce01808ca6b
📒 Files selected for processing (4)
src/agent/channel_history.rssrc/main.rssrc/messaging/webchat.rssrc/tools/reply.rs
- Extract duplicated card-flattening logic into OutboundResponse::text_with_cards() in src/lib.rs - Add warn logging for card deserialization failures in channel_history.rs (was silently swallowed by .ok()) - Fix inaccurate comments: main.rs referenced "webchat frontend" but serves all SSE consumers; channel_history.rs referenced "webchat frontend" but feeds in-memory LLM history Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/agent/channel_history.rs (1)
172-194:⚠️ Potential issue | 🟠 MajorCard extraction is still gated on
contentbeing a string.On Line 172–174, the function still won’t preserve card-only reply payloads when
contentis absent or non-string. Parse cards independently, then merge whichever parts exist.Suggested fix
- if let Some(content_value) = tool_call.function.arguments.get("content") - && let Some(text) = content_value.as_str() - { - // Also extract card descriptions so the full response - // (not just the short content text) is preserved in - // conversation history for subsequent LLM turns. - let cards = match tool_call.function.arguments.get("cards") { - Some(v) => { - serde_json::from_value::<Vec<crate::Card>>(v.clone()) - .unwrap_or_else(|e| { - tracing::warn!( - error = %e, - "failed to deserialize cards from cancelled reply tool call; \ - card content will be omitted from history" - ); - Vec::new() - }) - } - None => Vec::new(), - }; - - return Some(crate::OutboundResponse::text_with_cards(text, &cards)); - } + let text = tool_call + .function + .arguments + .get("content") + .and_then(|value| value.as_str()) + .unwrap_or(""); + + let cards = match tool_call.function.arguments.get("cards") { + Some(value) => serde_json::from_value::<Vec<crate::Card>>(value.clone()) + .unwrap_or_else(|error| { + tracing::warn!( + error = %error, + "failed to deserialize cards from cancelled reply tool call; \ + card content will be omitted from history" + ); + Vec::new() + }), + None => Vec::new(), + }; + + let merged = crate::OutboundResponse::text_with_cards(text, &cards); + if !merged.trim().is_empty() { + return Some(merged); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/agent/channel_history.rs` around lines 172 - 194, The current logic only extracts cards when a string "content" exists, so card-only replies are dropped; update the branch in the function handling tool_call.function.arguments to parse cards unconditionally (use the existing serde_json::from_value::<Vec<crate::Card>>(...) logic against arguments.get("cards") regardless of "content"), then build the outbound response by merging available parts: if content is a string use text, if cards parsed non-empty include them, and return crate::OutboundResponse::text_with_cards or the appropriate constructor with just cards/text as needed (preserve the current warning behavior on deserialize failure).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/agent/channel_history.rs`:
- Around line 172-194: The current logic only extracts cards when a string
"content" exists, so card-only replies are dropped; update the branch in the
function handling tool_call.function.arguments to parse cards unconditionally
(use the existing serde_json::from_value::<Vec<crate::Card>>(...) logic against
arguments.get("cards") regardless of "content"), then build the outbound
response by merging available parts: if content is a string use text, if cards
parsed non-empty include them, and return
crate::OutboundResponse::text_with_cards or the appropriate constructor with
just cards/text as needed (preserve the current warning behavior on deserialize
failure).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 23e37f50-3b81-4c03-85ba-c82821109a52
📒 Files selected for processing (5)
src/agent/channel_history.rssrc/lib.rssrc/main.rssrc/messaging/webchat.rssrc/tools/reply.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- src/messaging/webchat.rs
- src/main.rs
Summary
RichMessagevariantsOutboundResponse::text_from_cards()across all webchat paths: SSE event forwarding (main.rs), broadcast (webchat.rs), conversation logging (reply.rs), and cancelled-history extraction (channel_history.rs)Test plan
textfield)🤖 Generated with Claude Code