View toolcall details #1398
View toolcall details #1398
Conversation
…-toolcall-details
…-toolcall-details
📝 WalkthroughWalkthroughAdds a nullable tool_details field across DB schema, server types/ops, JS bindings, and frontend. Parses and surfaces tool details in chat messages (tool-start), rendering them via an Accordion UI when present. Extends streaming stop condition from 3 to 5 steps. Seeds and inserts updated messages with tool_details persisted and parsed. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI
participant useChatLogic
participant DB
participant Tool
User->>UI: Sends prompt
UI->>useChatLogic: start stream
useChatLogic->>DB: upsert user text (tool_details: null)
useChatLogic->>Tool: tool-call initiated
Tool-->>useChatLogic: tool-start { input }
useChatLogic->>DB: upsert tool-start (tool_details: JSON(input))
useChatLogic-->>UI: emit message { type: tool-start, toolDetails: parsed input }
Tool-->>useChatLogic: tool-result / tool-error
useChatLogic->>DB: upsert tool-result/error (tool_details: null)
useChatLogic->>DB: upsert AI text deltas (tool_details: null)
Note over useChatLogic: stopWhen stepCountIs(5)
UI->>DB: fetch messages
DB-->>UI: rows incl. tool_details
UI->>useChatQueries: map rows
useChatQueries-->>UI: parse tool_details for tool-start -> message.toolDetails
UI-->>User: render Accordion for tool-start with details
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
| message.role.to_string(), | ||
| message.content, | ||
| message.r#type.to_string(), | ||
| message.tool_details.unwrap_or_default(), |
There was a problem hiding this comment.
Inserting empty string for absent tool_details loses null semantics and can break queries expecting NULL; pass SQL NULL when the Option is None for consistency with schema and existing patterns.
(Based on your team's feedback about ensuring Option fields map to SQL NULL instead of empty strings for accurate database semantics.)
Prompt for AI agents
Address the following comment on crates/db-user/src/chat_messages_ops.rs at line 29:
<comment>Inserting empty string for absent tool_details loses null semantics and can break queries expecting NULL; pass SQL NULL when the Option is None for consistency with schema and existing patterns.
(Based on your team's feedback about ensuring Option fields map to SQL NULL instead of empty strings for accurate database semantics.)</comment>
<file context>
@@ -25,6 +26,7 @@ impl UserDatabase {
message.role.to_string(),
message.content,
message.r#type.to_string(),
+ message.tool_details.unwrap_or_default(),
],
)
</file context>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/desktop/src/components/right-panel/hooks/useChatLogic.ts (1)
444-464: JSON.stringify on tool input can throw (e.g., circular structures) — guard it
chunk.inputmay be non-serializable. A thrown error here would break the stream. Wrap stringify to fail closed.Apply within this hunk:
- tool_details: JSON.stringify(chunk.input), + tool_details: (() => { + try { + return JSON.stringify(chunk.input); + } catch { + return null; // avoid crashing persistence on bad input + } + })(),
🧹 Nitpick comments (9)
crates/db-user/src/chat_messages_migration_2.sql (1)
1-4: Optional JSON validity enforcement
Our codebase already uses JSON1 functions—e.g.json_extractappears incrates/db-admin/src/billings_ops.rs—so the SQLite build has JSON1 enabled. You can safely add aCHECKconstraint to ensure only valid JSON is stored intool_details.Please update
crates/db-user/src/chat_messages_migration_2.sqlas follows:ALTER TABLE chat_messages - COLUMN tool_details TEXT DEFAULT NULL; + COLUMN tool_details TEXT + DEFAULT NULL + CHECK ( + tool_details IS NULL + OR json_valid(tool_details) + );This is an optional refactor to harden data integrity at the database layer.
crates/db-user/src/chat_messages_types.rs (1)
37-38: Avoid double-serialization; consider a typed representation.If
tool_detailsalways contains JSON, representing it asOption<serde_json::Value>(or a typed struct) would prevent stringly-typed handling and repeated parse/format cycles across layers.Example:
- pub tool_details: Option<String>, + pub tool_details: Option<serde_json::Value>,This change would require:
- Updating insert/query bindings to serialize/deserialize JSON.
- Returning typed data to TS bindings (or still stringify at the boundary if needed).
crates/db-user/src/init.rs (1)
770-792: Optionally seed one tool-start example with realistic tool details.This helps validate UI rendering and end-to-end JSON parsing without manual setup.
For example (add alongside
chat_message_1):let chat_message_tool = ChatMessage { id: uuid::Uuid::new_v4().to_string(), group_id: chat_group_1.id.clone(), role: ChatMessageRole::Assistant, content: "Running tool: web.search".to_string(), created_at: now, r#type: ChatMessageType::ToolStart, tool_details: Some(serde_json::json!({ "tool": "web.search", "args": { "q": "hyprnote" }, "callId": "call_1", "startedAt": now }).to_string()), }; let _ = db.upsert_chat_message(chat_message_tool).await?;apps/desktop/src/components/right-panel/components/chat/types.ts (1)
14-15: Replaceanywith a safer type (unknownor a dedicatedToolDetails).
anydefeats type safety and makes downstream usage opaque. At minimum, preferunknown; ideally define a narrow interface for the details you render.Apply minimal change:
- toolDetails?: any; + toolDetails?: unknown;Or introduce a type:
+export type ToolDetails = { + tool?: string; + callId?: string; + args?: unknown; + status?: "running" | "done" | "error"; + error?: string; + startedAt?: string; + finishedAt?: string; + [key: string]: unknown; +}; ... - toolDetails?: any; + toolDetails?: ToolDetails;apps/desktop/src/components/right-panel/hooks/useChatQueries.ts (1)
89-98: Harden tool_details parsing; avoid noisy logs and preserve fallback valueCurrent IIFE logs on every parse failure and drops the raw value. Prefer a quiet, reusable parser that returns the original string when JSON.parse fails so the UI can still show something. This also keeps comments minimal per guidelines.
Apply within this hunk:
- toolDetails: msg.type === "tool-start" && msg.tool_details - ? (() => { - try { - return JSON.parse(msg.tool_details); - } catch (error) { - console.error("Failed to parse tool_details for tool-start:", msg.id, error); - return undefined; - } - })() - : undefined, + toolDetails: + msg.type === "tool-start" && typeof msg.tool_details === "string" + ? (msg.tool_details.trim() + ? safeParseToolDetails(msg.tool_details) + : undefined) + : undefined,Add once in this file (outside the hunk) to support the change:
function safeParseToolDetails(raw: string): unknown { const s = raw.trim(); if (!s) return undefined; if (s.startsWith("{") || s.startsWith("[")) { try { return JSON.parse(s); } catch { return s; // fall back to raw for rendering } } return s; }apps/desktop/src/components/right-panel/hooks/useChatLogic.ts (2)
423-434: Persisted AI text uses current time — potential ordering drift vs. in-memory message timestampUsing
new Date().toISOString()here can reorder messages relative to the already-emitted in-memory AI text message on reload (DB is ordered bycreated_at). Consider reusing the original message timestamp used when the “text-delta” message was created.If you choose to fix now, capture the timestamp at creation and reuse it here when persisting.
515-525: Final AI text persist mirrors earlier timing concernAs above, reusing a stored timestamp (instead of
new Date()) avoids ordering drift on reload.apps/desktop/src/components/right-panel/components/chat/message-content.tsx (1)
16-60: ToolDetailsRenderer: small resilience and typing nits
- Prefer
unknownoveranyand refine at render site.- Consider treating empty strings as “no details” but preserve non-empty strings (already handled if upstream returns string on parse failure per earlier suggestion).
Apply within this hunk:
-function ToolDetailsRenderer({ details }: { details: any }) { +function ToolDetailsRenderer({ details }: { details: unknown }) { if (!details) { @@ - {typeof details === 'object' ? JSON.stringify(details, null, 2) : String(details)} + {typeof details === "object" ? JSON.stringify(details, null, 2) : String(details)}crates/db-user/src/chat_messages_ops.rs (1)
18-21: Store NULL instead of empty string for absent tool_details
unwrap_or_default()writes"", which semantically differs fromNULLand can cause subtle TS/serde mismatches later. You can coerce empty string to NULL at write time withNULLIF.Apply within this hunk:
- type, - tool_details - ) VALUES (?, ?, ?, ?, ?, ?, ?) + type, + tool_details + ) VALUES (?, ?, ?, ?, ?, ?, NULLIF(?, ''))Keep the parameter line as-is (passing the possibly-empty string), letting SQLite store NULL when empty.
If you prefer preserving true NULLs end-to-end, we can also adjust the parameter type to pass a DB null directly. Happy to draft that if desired.
Also applies to: 29-29
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (10)
apps/desktop/src/components/right-panel/components/chat/message-content.tsx(3 hunks)apps/desktop/src/components/right-panel/components/chat/types.ts(1 hunks)apps/desktop/src/components/right-panel/hooks/useChatLogic.ts(9 hunks)apps/desktop/src/components/right-panel/hooks/useChatQueries.ts(1 hunks)crates/db-user/src/chat_messages_migration_2.sql(1 hunks)crates/db-user/src/chat_messages_ops.rs(2 hunks)crates/db-user/src/chat_messages_types.rs(1 hunks)crates/db-user/src/init.rs(1 hunks)crates/db-user/src/lib.rs(2 hunks)plugins/db/js/bindings.gen.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{js,ts,tsx,rs}
⚙️ CodeRabbit configuration file
**/*.{js,ts,tsx,rs}: 1. No error handling.
2. No unused imports, variables, or functions.
3. For comments, keep it minimal. It should be about "Why", not "What".
Files:
apps/desktop/src/components/right-panel/components/chat/types.tscrates/db-user/src/chat_messages_types.rsplugins/db/js/bindings.gen.tsapps/desktop/src/components/right-panel/hooks/useChatQueries.tscrates/db-user/src/lib.rsapps/desktop/src/components/right-panel/components/chat/message-content.tsxcrates/db-user/src/init.rsapps/desktop/src/components/right-panel/hooks/useChatLogic.tscrates/db-user/src/chat_messages_ops.rs
🧬 Code graph analysis (2)
apps/desktop/src/components/right-panel/components/chat/message-content.tsx (1)
packages/ui/src/components/ui/accordion.tsx (4)
Accordion(56-56)AccordionItem(56-56)AccordionTrigger(56-56)AccordionContent(56-56)
apps/desktop/src/components/right-panel/hooks/useChatLogic.ts (1)
packages/utils/src/ai.ts (1)
stepCountIs(16-16)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: cubic · AI code reviewer
- GitHub Check: ci (windows, windows-latest)
- GitHub Check: ci (macos, macos-latest)
🔇 Additional comments (15)
crates/db-user/src/chat_messages_migration_2.sql (1)
1-4: Schema addition is straightforward and backward-compatible.Nullable TEXT with no backfill risk looks good. Append-only migration aligns with existing pattern.
crates/db-user/src/chat_messages_types.rs (2)
37-38: Field addition aligns with the migration; type is appropriate for nullable column.No behavioral risk here; derives keep bindings generation intact.
37-38: AllChatMessageconstructors now specifytool_details.
crates/db-user/src/init.rs:781includes
tool_details: NoneNo other explicit
ChatMessage { … }initializations were found.crates/db-user/src/init.rs (1)
781-792: Seed value usesNone— good default.Keeps old seed behavior and exercises nullability.
crates/db-user/src/lib.rs (2)
132-157: Migration wiring looks correct and append-only.Array length updated to 24 and new migration appended at the end — consistent with the "Append only" guidance.
132-157: MIGRATIONS array length verified
- Declared length (
&str; 24) matches the number ofinclude_str!entries (24)- All referenced SQL files (including
chat_messages_migration_2.sql) are present undercrates/db-user/src/No merge skew detected—this comment can be resolved.
apps/desktop/src/components/right-panel/components/chat/types.ts (1)
14-15: JSON parsing fortoolDetailsis safely gatedThe
JSON.parse(msg.tool_details)invocation inapps/desktop/src/components/right-panel/hooks/useChatQueries.tsis only reached when:
msg.type === "tool-start"msg.tool_detailsis truthy (i.e. non-null, non-empty)
It’s also wrapped in atry/catchthat logs parse errors and returnsundefinedon failure.No further changes are needed.
plugins/db/js/bindings.gen.ts (1)
165-165: Bindings updated with tool_details — looks consistentThe generated ChatMessage type now includes
tool_details: string | null, matching the RustOption<String>and frontend usage. No concerns here.apps/desktop/src/components/right-panel/hooks/useChatLogic.ts (5)
160-166: DB write includes tool_details: null for user messages — consistent with schemaSetting
tool_details: nullfor user “text-delta” aligns with the new column and keeps semantics clear.
480-488: Tool result persisted with tool_details: null — consistentMatches the contract that only tool-start carries details. LGTM.
502-510: Tool error persisted with tool_details: null — consistentSame pattern as tool-result. LGTM.
573-581: Error-path AI message uses tool_details: null — consistentError fallback write aligns with the new schema. LGTM.
345-348: stopWhen increment to 5 is isolated — no additional dependencies detectedI searched the codebase for any other calls to
stepCountIsor uses ofstopWhenand found only the single instance on line 344 ofapps/desktop/src/components/right-panel/hooks/useChatLogic.ts. There are no other occurrences or logic paths assuming a step count of 3, so raising it from 3 to 5 is self-contained and should not introduce unintended stalls or regressions.apps/desktop/src/components/right-panel/components/chat/message-content.tsx (2)
3-3: Import of Accordion components — OKImports match usage below and local UI package. LGTM.
174-238: Accordion-based tool-start details UX — looks goodThe “tool-start” path renders compact-by-default with expandable details and keeps the no-details fallback. Nicely scoped styles and minimal surface area.
| ALTER TABLE | ||
| chat_messages | ||
| ADD | ||
| COLUMN tool_details TEXT DEFAULT NULL; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Ensure we persist NULL (not empty string) when there are no tool details.
Downstream code appears to JSON-parse this field; an empty string would throw. Please confirm that inserts bind NULL for missing values rather than "".
If chat_messages_ops.rs currently uses unwrap_or_default(), prefer passing the Option through so the driver writes NULL:
// Before (risk: "" gets stored)
tool_details: message.tool_details.unwrap_or_default(),
// After (store NULL when absent)
tool_details: message.tool_details.as_deref()Run to verify call sites:
🏁 Script executed:
#!/bin/bash
rg -n "tool_details.*unwrap_or_default\(\)|unwrap_or\(\"\"\)" -C2
rg -n "INSERT.*chat_messages" -C3 crates/db-user/src/chat_messages_ops.rsLength of output: 3741
Persist NULL for missing tool_details instead of empty string
The current implementation in crates/db-user/src/chat_messages_ops.rs binds an empty string when message.tool_details is None, causing '' to be stored rather than SQL NULL. Downstream JSON parsing will fail on an empty string. Please update the binding to propagate the Option so that the driver writes NULL when there are no tool details.
• Location: crates/db-user/src/chat_messages_ops.rs:29
• Current code binds unwrap_or_default(), which is "" for None.
• Change to as_deref() to pass Option<&str> through.
Suggested diff:
- &message.tool_details.unwrap_or_default(),
+ &message.tool_details.as_deref(),This ensures:
Some(s)→ bindssNone→ binds SQLNULL
🤖 Prompt for AI Agents
In crates/db-user/src/chat_messages_ops.rs around line 29, the code currently
binds message.tool_details using unwrap_or_default() which writes an empty
string instead of SQL NULL; replace the unwrap_or_default() call with as_deref()
so you pass an Option<&str> to the query binder (Some(s) binds s, None binds SQL
NULL). Ensure the prepared statement parameter type accepts Option<&str> (no
further transformation) and remove any .unwrap_or_default() usage for
tool_details so nulls are persisted.
Summary by cubic
Show tool call input details in chat. Tool-start messages now have an expandable view that pretty-prints the tool’s input, with data persisted in the DB.
New Features
Migration