Add debug logger and tool call support where OpenAI compatible API supports tool calling#5
Add debug logger and tool call support where OpenAI compatible API supports tool calling#5
Conversation
WalkthroughThe changes introduce structured support for tool/function calls within chat messages, extending database schemas, message types, and UI rendering to handle tool call metadata and results. New modules implement tool execution logic, including a web search tool, and utilities for normalizing and validating tool call data. Additional logging, prompt updates, and project rules for Zod imports are included. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI
participant Backend
participant ToolExecutor
participant DB
User->>UI: Send message
UI->>Backend: Submit message
Backend->>DB: Store user message
Backend->>Backend: Generate assistant response
alt Assistant calls tool(s)
Backend->>ToolExecutor: Execute tool(s) (async)
ToolExecutor->>Backend: Return tool result(s)
Backend->>DB: Store tool call result(s) as "tool" messages
Backend->>Backend: Continue conversation (recursion, depth check)
end
Backend->>DB: Store assistant message (with tool_calls)
Backend->>UI: Stream response (assistant + tool results)
UI->>User: Render message, tool calls, tool results (collapsible)
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Nitpick comments (8)
src/db/schema.ts (1)
87-88: Well-designed tool call columns.The implementation is solid with appropriate data types and defaults. Consider refining the comment for clarity.
- tool_calls: jsonb("tool_calls").$defaultFn(() => []), // An array/object of JSON is apparently 'jsonb' format and not type 'array' + tool_calls: jsonb("tool_calls").$defaultFn(() => []), // Array of tool call objects stored as jsonbsrc/tools/debugLogger.ts (1)
1-41: Well-designed debug logging utility with room for improvement.The overloaded function design provides good flexibility, and environment-based logging is useful for development. Consider these enhancements for better robustness:
export function debugLogger( - environmentsOrMessage: string | string[] | any, + environmentsOrMessage: string | string[] | unknown, messageOrArg?: any, ...restArgs: any[] ): void { let environments: string[]; - let message: any; + let message: unknown; let args: any[]; + // Validate environments parameter if (Array.isArray(environmentsOrMessage)) { + if (!environmentsOrMessage.every(env => typeof env === 'string')) { + throw new Error('All environment values must be strings'); + } environments = environmentsOrMessage; message = messageOrArg; args = restArgs; } else if (typeof environmentsOrMessage === 'string' && messageOrArg !== undefined) { environments = [environmentsOrMessage]; message = messageOrArg; args = restArgs; } else { - // First param is the message (default to 'development') - environments = ['development']; + // First param is the message (log in all environments if NODE_ENV not set) + environments = process.env.NODE_ENV ? [process.env.NODE_ENV] : ['development']; message = environmentsOrMessage; args = messageOrArg !== undefined ? [messageOrArg, ...restArgs] : restArgs; } const nodeEnv = process.env.NODE_ENV || 'development'; if (environments.includes(nodeEnv)) { console.log(message, ...args); } }client/src/components/MessageRenderer.tsx (5)
77-94: Extract message parsing logic to a utility function.The message transformation logic is complex and could be reused. Consider extracting it to improve maintainability.
Create a utility function:
// In a separate utility file export function parseApiMessage(msg: any): Message { return { ...msg, tool_calls: msg.tool_calls ? msg.tool_calls.map((tc: any) => ({ ...tc, function: { ...tc.function, arguments: typeof tc.function.arguments === 'string' ? tc.function.arguments : JSON.stringify(tc.function.arguments) } })) : null, toolCallId: msg.toolCallId || null, role: ["system", "user", "assistant", "tool"].includes(msg.role) ? msg.role : "assistant" }; }
308-308: Clean up code comments and improve structure.Remove self-deprecating comments and improve code organization for better maintainability.
- {/* Only wrap in space-y-2 for non-user messages EDIT: changed wrapper to use space-y-3*/} + {/* Render message content based on role */} ... - /* Assistant/system/tool messages: apply prose only to final content */ - /* Nevermind we use space-y-3 for more consistiency because my code is cooked */ + /* Non-user messages with consistent spacing */Also applies to: 333-333
369-403: Consider extracting tool rendering logic to separate components.The nested ternary operators and complex rendering logic would benefit from being split into smaller, focused components.
// Create separate components const ToolCallsDisplay = ({ toolCalls, showFunctionCalls, onToggle }: Props) => ( <Collapsible> <CollapsibleTrigger className="flex items-center gap-1 transition-all text-foreground/50 hover:text-foreground" onClick={onToggle} > {showFunctionCalls ? <ChevronDown /> : <ChevronRight />} Function Calls ({toolCalls.length}) </CollapsibleTrigger> <CollapsibleContent className="mt-2"> <div className="border-l-2 border-foreground/20 pl-3 space-y-2"> {toolCalls.map((toolCall, index) => ( <ToolCallItem key={toolCall.id || index} toolCall={toolCall} /> ))} </div> </CollapsibleContent> </Collapsible> ); const ToolCallItem = ({ toolCall }: { toolCall: any }) => ( <div className="text-sm"> <div className="font-mono text-foreground/80 mb-1"> {toolCall.function.name} </div> <div className="text-xs text-foreground/60 bg-muted p-2 rounded font-mono overflow-x-auto"> {toolCall.function.arguments} </div> </div> );
333-334: Remove unprofessional comment.The comment "Nevermind we use space-y-3 for more consistiency because my code is cooked" is unprofessional and should be removed or rewritten.
- /* Assistant/system/tool messages: apply prose only to final content */ - /* Nevermind we use space-y-3 for more consistiency because my code is cooked */ + /* Assistant/system/tool messages: apply consistent spacing */
407-407: Improve readability of finish_reason validation.The current condition with
!== trueis confusing. Consider refactoring for clarity.- {message.finish_reason && ["stop", "tool_calls", "tool_calls_response"].includes(message.finish_reason) !== true ? ( + {message.finish_reason && !["stop", "tool_calls", "tool_calls_response"].includes(message.finish_reason) ? (src/sync.ts (1)
343-343: Use a more descriptive senderId for tool responses.The hardcoded "tool_response" senderId could be more informative.
- senderId: "tool_response", // Ensure senderId is passed correctly + senderId: `tool:${toolCall.function.name}`, // More descriptive sender ID
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
client/src/components/MessageRenderer.tsx(5 hunks)client/src/lib/db.ts(1 hunks)drizzle/meta/_journal.json(1 hunks)src/chats.ts(1 hunks)src/db/schema.ts(3 hunks)src/sync.ts(7 hunks)src/tools/debugLogger.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
client/src/components/MessageRenderer.tsx (2)
client/src/lib/db.ts (2)
Message(10-29)Message(30-30)client/src/components/ui/collapsible.tsx (3)
Collapsible(31-31)CollapsibleTrigger(31-31)CollapsibleContent(31-31)
src/sync.ts (3)
src/tools/debugLogger.ts (1)
devLog(44-44)src/db/index.ts (1)
db(6-6)src/db/schema.ts (1)
chatMessages(73-97)
🔇 Additional comments (16)
drizzle/meta/_journal.json (1)
11-39: LGTM! Migration entries are properly structured.The new migration entries follow standard Drizzle conventions with proper incremental indexing, unique timestamps, and consistent version numbering.
src/db/schema.ts (2)
1-1: Good addition of jsonb import.Adding jsonb import supports the new tool_calls column functionality.
72-72: Excellent addition of "tool" role.Adding "tool" to the role enum aligns perfectly with OpenAI's tool calling API specification and enables proper tool response handling.
src/tools/debugLogger.ts (1)
43-44: Good convenience wrapper.The devLog wrapper provides a clean interface for development-only logging.
src/chats.ts (2)
99-100: Excellent consistent handling of new tool call properties.The conditional assignment logic properly handles null/undefined values from the database while ensuring consistent message structure.
103-108: Great consistency across both message code paths.Both file and non-file message paths now consistently include the new tool call properties with proper null handling.
client/src/lib/db.ts (2)
12-12: Perfect schema alignment with backend.Adding "tool" to the role enum maintains consistency with the backend schema changes.
20-28: Excellent tool calls schema structure.The schema perfectly matches OpenAI's tool calling API specification with proper validation:
- Correct structure with id, type, and function properties
- Appropriate use of z.literal("function") for type validation
- Proper nullable handling for optional fields
client/src/components/MessageRenderer.tsx (4)
183-184: LGTM!The state variables for managing collapsible sections are appropriately named and initialized.
407-407: LGTM!The finish reason check correctly includes the new tool-related finish reasons.
124-125: LGTM!The addition of
tool_callsandtoolCallIdfields with null values maintains consistency with the updated Message schema.Also applies to: 141-142
183-184: LGTM!The state variables for managing collapsible sections are well-named and follow the existing pattern.
src/sync.ts (4)
16-16: LGTM!The type extensions properly support the new tool message functionality.
Also applies to: 21-22
273-273: LGTM!Proper use of the debug logging utility.
251-262: LGTM!The message role handling correctly supports tool messages and preserves tool_calls in assistant messages.
160-160: LGTM!The tool call accumulation logic correctly handles streaming chunks from the OpenAI API.
Also applies to: 297-317
…put 'reasoning' section for now
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
client/src/components/MessageRenderer.tsx (2)
67-67: Replaceanytype with proper TypeScript interface for better type safety.This is a duplicate of a previously raised concern. The use of
anyreduces type safety and makes the code more error-prone.
90-92: Handle invalid roles gracefully instead of throwing errors.This is a duplicate of a previously raised critical concern. Throwing an error in a map function will crash the component.
🧹 Nitpick comments (1)
client/src/components/MessageRenderer.tsx (1)
73-95: Consider extracting the complex message parsing logic into a separate utility function.The message transformation logic handles multiple concerns (tool calls normalization, role validation, argument string conversion) and could benefit from extraction for better maintainability and testability.
+// Extract to a separate utility function +function normalizeApiMessage(msg: any) { + return { + ...msg, + tool_calls: msg.tool_calls ? msg.tool_calls.map((tc: any) => ({ + ...tc, + function: { + ...tc.function, + arguments: typeof tc.function.arguments === 'string' + ? tc.function.arguments + : JSON.stringify(tc.function.arguments) + } + })) : null, + toolCallId: msg.toolCallId || null, + role: ["system", "user", "assistant", "tool"].includes(msg.role) + ? msg.role + : (() => { throw new Error(`Invalid role: ${msg.role}`) })() + }; +} - const parsedMessages = messages.messages?.map((msg: any) => ({ - ...msg, - tool_calls: msg.tool_calls ? msg.tool_calls.map((tc: any) => ({ - ...tc, - function: { - ...tc.function, - arguments: typeof tc.function.arguments === 'string' - ? tc.function.arguments - : JSON.stringify(tc.function.arguments) - } - })) : null, - toolCallId: msg.toolCallId || null, - // Throw error if role invalid - role: ["system", "user", "assistant", "tool"].includes(msg.role) - ? msg.role - : (() => { throw new Error(`Invalid role: ${msg.role}`) })() - })) || []; + const parsedMessages = messages.messages?.map(normalizeApiMessage) || [];
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
client/src/components/MessageRenderer.tsx(5 hunks)
🔇 Additional comments (4)
client/src/components/MessageRenderer.tsx (4)
119-121: LGTM! Consistent addition of tool call fields to pending messages.The tool_calls and toolCallId fields are correctly added with null defaults to maintain schema consistency for pending messages.
Also applies to: 136-138
178-179: LGTM! Appropriate state management for tool UI controls.The new state variables for controlling tool result and function call collapsible sections follow the existing pattern established for the thinking section.
329-382: LGTM! Well-structured tool call rendering implementation.The rendering logic properly handles different message types:
- Tool messages with collapsible results and optional toolCallId display
- Messages with tool_calls showing function names and arguments
- Fallback to normal message rendering
The UI design is consistent with existing collapsible sections and provides good visual hierarchy.
385-385: LGTM! Appropriate finish_reason filtering for tool calls.The condition correctly excludes "tool_calls" and "tool_calls_response" from showing alerts, which is appropriate since these are normal operation states for tool calling.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
client/src/components/MessageRenderer.tsx (1)
307-389: Consider extracting message type renderers for better maintainability.The rendering logic correctly handles different message types, but the nested conditional structure is complex. Consider extracting separate renderer functions.
+function renderUserMessage(message: Message, retryVariables?: string) { + return ( + <div className="prose"> + <MarkdownRenderer>{retryVariables ?? message.message}</MarkdownRenderer> + </div> + ); +} + +function renderToolMessage(message: Message, showToolResult: boolean, setShowToolResult: (show: boolean) => void, retryVariables?: string) { + return ( + <Collapsible> + <CollapsibleTrigger + className="flex items-center gap-1 transition-all text-foreground/50 hover:text-foreground" + onClick={() => setShowToolResult(!showToolResult)} + > + {showToolResult ? <ChevronDown /> : <ChevronRight />} Tool Result + {message.toolCallId && ( + <span className="text-xs text-foreground/40">({message.toolCallId})</span> + )} + </CollapsibleTrigger> + <CollapsibleContent className="mt-2"> + <div className="border-l-2 border-foreground/20 pl-3 prose"> + <MarkdownRenderer>{retryVariables ?? message.message}</MarkdownRenderer> + </div> + </CollapsibleContent> + </Collapsible> + ); +} {message.role === "user" ? ( - /* User messages: apply prose only to final content */ - <div className="prose"> - <MarkdownRenderer>{retryMessage.variables ?? message.message}</MarkdownRenderer> - </div> + renderUserMessage(message, retryMessage.variables) ) : ( /* Assistant/system/tool messages: ... */ <div className="space-y-3"> {/* ... reasoning section ... */} - {/* Tool message rendering */} - {message.role === "tool" ? ( - <Collapsible> - {/* ... tool message content ... */} - </Collapsible> + {message.role === "tool" ? ( + renderToolMessage(message, showToolResult, setShowToolResult, retryMessage.variables) ) : /* ... rest of logic ... */}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
client/src/components/MessageRenderer.tsx(5 hunks)
🧰 Additional context used
🧠 Learnings (1)
client/src/components/MessageRenderer.tsx (1)
Learnt from: Pablonara
PR: t35-turbo/TanTanChat#5
File: client/src/components/MessageRenderer.tsx:0-0
Timestamp: 2025-07-01T22:16:48.218Z
Learning: In TanTanChat MessageRenderer component, user messages do not generate reasoning output, so reasoning display logic should only exist for assistant/system/tool messages, not user messages.
🔇 Additional comments (3)
client/src/components/MessageRenderer.tsx (3)
125-126: LGTM: Proper initialization of tool call fields for pending messages.The addition of
tool_calls: nullandtoolCallId: nullensures pending messages conform to the Message schema requirements.Also applies to: 142-143
184-185: LGTM: Clear state variables for UI collapsible sections.The state variables are well-named and follow React conventions for managing collapsible UI sections.
391-391: LGTM: Proper exclusion of tool call finish reasons from error alerts.The updated condition correctly prevents tool call related finish reasons ("tool_calls", "tool_calls_response") from being displayed as errors, which is appropriate for normal tool calling workflows.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (5)
src/sync.ts (5)
83-94: Duplicate issue: Add comprehensive error handling for the API call.This issue was already identified in previous reviews - the function lacks proper error handling for network failures and response validation.
333-337: Duplicate issue: Fix hardcoded parameter assumption.This issue was already identified in previous reviews - the code incorrectly assumes all tool functions accept a
search_termsparameter.
399-400: Duplicate issue: Add recursion depth limit.This issue was already identified in previous reviews - the recursive call to
newMessagecould lead to infinite loops.
509-509: Duplicate issue: Use devLog instead of console.log.This issue was already identified in previous reviews - should use devLog for consistency.
512-519: Duplicate issue: Add error handling for JSON parsing.This issue was already identified in previous reviews - the JSON.parse operation needs proper error handling.
🧹 Nitpick comments (1)
src/sync.ts (1)
340-351: Consider using a transaction for database operations.The database insertion for tool responses should be wrapped in a transaction to ensure consistency, especially when multiple tool calls are processed.
- //add to db - await db.insert(chatMessages).values({ + // Add to db within transaction + await db.transaction(async (tx) => { + await tx.insert(chatMessages).values({ id: crypto.randomUUID(), chatId, senderId: "tool_response", // Ensure senderId is passed correctly role: "tool", message: JSON.stringify(result), toolCallId: toolCall.id, reasoning: "", tool_calls: [], finish_reason: "tool_calls_response", createdAt: new Date(), - }); + }); + });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/sync.ts(7 hunks)
🧰 Additional context used
🧠 Learnings (1)
src/sync.ts (1)
Learnt from: Pablonara
PR: t35-turbo/TanTanChat#5
File: client/src/components/MessageRenderer.tsx:0-0
Timestamp: 2025-07-01T22:16:48.218Z
Learning: In TanTanChat MessageRenderer component, user messages do not generate reasoning output, so reasoning display logic should only exist for assistant/system/tool messages, not user messages.
🔇 Additional comments (4)
src/sync.ts (4)
159-160: LGTM! Tool call accumulator properly initialized.The accumulator for tool calls is correctly initialized to handle streaming chunks from the OpenAI API.
251-268: Well-structured message role handling for tool support.The message mapping logic correctly handles the new "tool" role and preserves tool call data for assistant messages. The conditional structure properly handles all message types.
273-273: LGTM! Consistent use of debug logging.Using devLog for debug output maintains consistency with the project's logging approach.
294-325: Robust tool call chunk accumulation logic.The streaming chunk accumulation correctly handles partial tool call data by index, properly initializing and concatenating arguments. The logic properly handles missing chunks and maintains tool call structure.
…nd also make arguments in model response passed back as a JSON string and not actual JSON, which also fixes message regeneration
Pablonara
force-pushed
the
tool-call-support-oai
branch
from
8baea91 to
ea434dc
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
src/sync.ts (5)
83-94: Acknowledged: Testing placeholder function.Based on previous feedback, this is a testing placeholder function and doesn't require production-level error handling.
355-368: Fix the hardcoded parameter handling for tool functions.The current implementation still assumes all tools use
search_termsparameter, which is incorrect fortestSearchand brittle for future tools.- if (toolCall.function.name in TOOL_MAPPING) { - try { - const args = JSON.parse(toolCall.function.arguments); - let result; - switch (toolCall.function.name) { - case 'searchGutenbergBooks': - result = await TOOL_MAPPING.searchGutenbergBooks(args.search_terms); - break; - case 'testSearch': - result = await TOOL_MAPPING.testSearch(); - break; - default: - throw new Error(`Unknown tool function: ${toolCall.function.name}`); - } + if (toolCall.function.name in TOOL_MAPPING) { + try { + const args = JSON.parse(toolCall.function.arguments); + const toolFunction = TOOL_MAPPING[toolCall.function.name as keyof typeof TOOL_MAPPING]; + let result; + + // Handle different parameter patterns per tool + if (toolCall.function.name === 'searchGutenbergBooks') { + result = await toolFunction(args.search_terms); + } else if (toolCall.function.name === 'testSearch') { + result = await toolFunction(); + } else { + throw new Error(`Unknown tool function: ${toolCall.function.name}`); + }
394-413: Optimize database query to avoid race conditions.The current approach of fetching all messages and filtering in-memory could miss recently inserted tool messages due to timing issues.
- // First, fetch the tool response messages from the database - const toolResponseMessages = await db - .select() - .from(chatMessages) - .where(eq(chatMessages.chatId, chatId)) - .orderBy(chatMessages.createdAt); - - // Filter to get only the tool messages we just created - const recentToolMessages = toolResponseMessages - .filter(msg => msg.role === "tool" && - accumulatedToolCalls.some(tc => tc.id === msg.toolCallId)) + // Fetch only the specific tool messages we just created + const toolCallIds = accumulatedToolCalls.map(tc => tc.id); + const recentToolMessages = await db + .select() + .from(chatMessages) + .where( + and( + eq(chatMessages.chatId, chatId), + eq(chatMessages.role, "tool"), + inArray(chatMessages.toolCallId, toolCallIds) + ) + ) .map(msg => ({ id: msg.id, role: "tool" as const, chatId: msg.chatId, senderId: msg.senderId, message: msg.message, createdAt: msg.createdAt, files: [], toolCallId: msg.toolCallId ?? undefined }));Note: You'll need to import
andandinArrayfromdrizzle-orm.
550-558: Add error handling for tool call argument parsing.The JSON parsing operation could fail silently, leading to runtime errors when the parsed arguments are used.
- // for some reason the tool call is not a real JSON object, so we need to parse it - const parsedToolCalls = tool_calls.map(call => ({ - ...call, - function: { - ...call.function, - arguments: typeof call.function.arguments === 'string' - ? JSON.parse(call.function.arguments) - : call.function.arguments, - }, - })); + // Parse tool call arguments with error handling + const parsedToolCalls = tool_calls.map(call => { + try { + return { + ...call, + function: { + ...call.function, + arguments: typeof call.function.arguments === 'string' + ? JSON.parse(call.function.arguments) + : call.function.arguments, + }, + }; + } catch (error) { + devLog("[Error] Failed to parse tool call arguments:", error); + return { + ...call, + function: { + ...call.function, + arguments: call.function.arguments, // Keep as string if parsing fails + }, + }; + } + });
534-543: Enhance debugging for tool call parsing failures.The error handling for JSON parsing could be improved with more context for debugging.
if (chunk.tool_calls) { try { + devLog("[Debug] Raw tool_calls chunk:", chunk.tool_calls); const chunkToolCalls = JSON.parse(chunk.tool_calls); if (Array.isArray(chunkToolCalls) && chunkToolCalls.length > 0) { tool_calls = chunkToolCalls; } } catch (error) { - devLog("[Error] Failed to parse tool calls:", error); + devLog("[Error] Failed to parse tool calls:", error, "Raw data:", chunk.tool_calls); } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/sync.ts(7 hunks)
🧰 Additional context used
🧠 Learnings (1)
src/sync.ts (2)
Learnt from: Pablonara
PR: t35-turbo/TanTanChat#5
File: src/sync.ts:83-94
Timestamp: 2025-07-02T00:29:04.108Z
Learning: The searchGutenbergBooks function in src/sync.ts is a placeholder tool for testing purposes, not production code requiring robust error handling.
Learnt from: Pablonara
PR: t35-turbo/TanTanChat#5
File: client/src/components/MessageRenderer.tsx:0-0
Timestamp: 2025-07-01T22:16:48.218Z
Learning: In TanTanChat MessageRenderer component, user messages do not generate reasoning output, so reasoning display logic should only exist for assistant/system/tool messages, not user messages.
🔇 Additional comments (3)
src/sync.ts (3)
12-12: LGTM: Debug logging import added.Good addition of the debug logging utilities to support the tool call functionality.
16-16: LGTM: Message type extended for tool support.The type extensions correctly add support for tool messages with the required fields.
Also applies to: 21-22
158-160: LGTM: Recursion depth protection implemented.Good implementation of recursion depth limiting to prevent infinite tool call loops, addressing the previous concern.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
src/sync.ts (3)
324-338: Add validation for tool call chunk indices.The code doesn't validate
toolCallChunk.indexbefore using it as an array index, which could lead to runtime errors with malformed data.This was previously flagged but the validation is still missing. The index should be validated to ensure it's a non-negative integer within expected bounds.
537-545: Add error handling for tool calls JSON parsing.The JSON.parse operation could fail if
chunk.tool_callscontains malformed JSON, but the error handling doesn't include the raw data for debugging.This issue was previously flagged and the current error handling is insufficient for debugging parsing failures.
552-560: Add error handling for tool call arguments parsing.The JSON.parse operation on
call.function.argumentscould fail if the arguments string is malformed, potentially breaking the entire tool call processing.const parsedToolCalls = tool_calls.map(call => ({ ...call, function: { ...call.function, - arguments: typeof call.function.arguments === 'string' - ? JSON.parse(call.function.arguments) - : call.function.arguments, + arguments: typeof call.function.arguments === 'string' + ? (() => { + try { + return JSON.parse(call.function.arguments); + } catch (error) { + devLog("[Error] Failed to parse tool call arguments:", error, "Raw data:", call.function.arguments); + return call.function.arguments; // Keep as string if parsing fails + } + })() + : call.function.arguments, }, }));
🧹 Nitpick comments (1)
src/sync.ts (1)
233-233: Consider using depth parameter in system prompt.The hardcoded
0in thedefault_promptcall might be better replaced with thedepthparameter to provide context about tool call recursion level.- content: default_prompt(opts.model.split("/")[0], opts.model.split("/")[0], 0) + "\n" + opts.system_prompt, + content: default_prompt(opts.model.split("/")[0], opts.model.split("/")[0], depth) + "\n" + opts.system_prompt,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/sync.ts(7 hunks)
🧰 Additional context used
🧠 Learnings (1)
src/sync.ts (2)
Learnt from: Pablonara
PR: t35-turbo/TanTanChat#5
File: src/sync.ts:83-94
Timestamp: 2025-07-02T00:29:04.108Z
Learning: The searchGutenbergBooks function in src/sync.ts is a placeholder tool for testing purposes, not production code requiring robust error handling.
Learnt from: Pablonara
PR: t35-turbo/TanTanChat#5
File: client/src/components/MessageRenderer.tsx:0-0
Timestamp: 2025-07-01T22:16:48.218Z
Learning: In TanTanChat MessageRenderer component, user messages do not generate reasoning output, so reasoning display logic should only exist for assistant/system/tool messages, not user messages.
🔇 Additional comments (2)
src/sync.ts (2)
355-368: LGTM: Improved tool execution with switch statement.The switch statement approach properly handles different tool functions with their specific argument requirements, addressing the previous hardcoded parameter assumption issue.
394-415: LGTM: Efficient database query for tool messages.The implementation now uses a specific database query with proper filtering instead of fetching all messages and filtering in-memory, addressing the potential race condition concern.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/sync.ts(7 hunks)
🧰 Additional context used
🧠 Learnings (1)
src/sync.ts (3)
Learnt from: Pablonara
PR: t35-turbo/TanTanChat#5
File: src/sync.ts:158-161
Timestamp: 2025-07-02T00:50:12.362Z
Learning: The searchGutenbergBooks and testSearch functions in src/sync.ts are placeholder tools for testing tool call functionality, not production code requiring robust error handling.
Learnt from: Pablonara
PR: t35-turbo/TanTanChat#5
File: src/sync.ts:83-94
Timestamp: 2025-07-02T00:29:04.108Z
Learning: The searchGutenbergBooks function in src/sync.ts is a placeholder tool for testing purposes, not production code requiring robust error handling.
Learnt from: Pablonara
PR: t35-turbo/TanTanChat#5
File: client/src/components/MessageRenderer.tsx:0-0
Timestamp: 2025-07-01T22:16:48.218Z
Learning: In TanTanChat MessageRenderer component, user messages do not generate reasoning output, so reasoning display logic should only exist for assistant/system/tool messages, not user messages.
🔇 Additional comments (6)
src/sync.ts (6)
5-5: LGTM! Type definitions properly support tool calling.The imports and type extensions correctly support the new tool calling functionality and align with the database schema updates.
Also applies to: 12-12, 16-16, 21-22
75-144: LGTM! Tool interface implementation is correct.The tool function definitions, OpenAI schemas, and mapping object properly implement the function calling interface for testing purposes.
255-290: LGTM! Message mapping correctly handles tool calls.The enhanced message mapping properly handles tool role messages and converts tool call arguments between formats as needed for OpenAI compatibility.
Also applies to: 295-295
163-164: Tool call accumulation logic is functional.The streaming tool call accumulation correctly handles chunked data and builds complete tool calls. Note that edge cases with invalid indices were previously identified but this implementation works for normal OpenAI streaming scenarios.
Also applies to: 316-339, 346-346
349-441: LGTM! Comprehensive tool call processing implementation.The tool call processing correctly handles execution, database storage, and conversation continuation. The switch statement approach properly handles different tool function signatures, and the recursive conversation continuation with depth increment is well-implemented.
531-570: LGTM! Redis subscriber properly handles tool calls.The tool call processing in the subscriber correctly parses streaming data, handles JSON conversion, and stores tool calls in the database with appropriate error handling and debug logging.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
client/src/lib/message-normalise.ts (1)
17-24: Simplify the role validation logic.The defensive programming approach and type safety are excellent. The logic can be simplified by using the
includes()method.export function safeRole(role: string): (typeof allowedRoles)[number] { - const idx = (allowedRoles as readonly string[]).indexOf(role); - if (idx !== -1) { - return allowedRoles[idx]; + if (allowedRoles.includes(role as any)) { + return role as (typeof allowedRoles)[number]; } console.warn(`Invalid role: ${role}, defaulting to 'assistant'`); return "assistant"; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
client/src/components/MessageRenderer.tsx(7 hunks)client/src/lib/message-normalise.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- client/src/components/MessageRenderer.tsx
🧰 Additional context used
🧠 Learnings (1)
client/src/lib/message-normalise.ts (1)
Learnt from: Pablonara
PR: t35-turbo/TanTanChat#5
File: src/sync.ts:324-338
Timestamp: 2025-07-02T01:04:58.995Z
Learning: In src/sync.ts, the tool call accumulation logic for toolCallChunk.index doesn't require additional validation because upstream components validate tool call data before it reaches the accumulation logic, preventing invalid indices from getting to this point.
| export function normaliseToolCalls(tool_calls?: any[] | null) { | ||
| if (!tool_calls) return null; | ||
| return tool_calls.map((tc) => ({ | ||
| ...tc, | ||
| function: { | ||
| ...tc.function, | ||
| arguments: | ||
| typeof tc.function.arguments === "string" | ||
| ? tc.function.arguments | ||
| : JSON.stringify(tc.function.arguments), | ||
| }, | ||
| })); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve type safety and add defensive programming.
The function logic is correct, but consider these improvements:
- Type safety: Replace
any[]with a more specific interface for better type checking - Null safety: Add defensive checks for
tc.functionto prevent runtime errors
+interface ToolCall {
+ function?: {
+ arguments?: string | object;
+ [key: string]: any;
+ };
+ [key: string]: any;
+}
+
-export function normaliseToolCalls(tool_calls?: any[] | null) {
+export function normaliseToolCalls(tool_calls?: ToolCall[] | null) {
if (!tool_calls) return null;
return tool_calls.map((tc) => ({
...tc,
function: {
- ...tc.function,
+ ...(tc.function || {}),
arguments:
- typeof tc.function.arguments === "string"
- ? tc.function.arguments
- : JSON.stringify(tc.function.arguments),
+ typeof tc.function?.arguments === "string"
+ ? tc.function.arguments
+ : JSON.stringify(tc.function?.arguments || {}),
},
}));
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export function normaliseToolCalls(tool_calls?: any[] | null) { | |
| if (!tool_calls) return null; | |
| return tool_calls.map((tc) => ({ | |
| ...tc, | |
| function: { | |
| ...tc.function, | |
| arguments: | |
| typeof tc.function.arguments === "string" | |
| ? tc.function.arguments | |
| : JSON.stringify(tc.function.arguments), | |
| }, | |
| })); | |
| } | |
| interface ToolCall { | |
| function?: { | |
| arguments?: string | object; | |
| [key: string]: any; | |
| }; | |
| [key: string]: any; | |
| } | |
| export function normaliseToolCalls(tool_calls?: ToolCall[] | null) { | |
| if (!tool_calls) return null; | |
| return tool_calls.map((tc) => ({ | |
| ...tc, | |
| function: { | |
| ...(tc.function || {}), | |
| arguments: | |
| typeof tc.function?.arguments === "string" | |
| ? tc.function.arguments | |
| : JSON.stringify(tc.function?.arguments || {}), | |
| }, | |
| })); | |
| } |
🤖 Prompt for AI Agents
In client/src/lib/message-normalise.ts lines 1 to 13, improve type safety by
defining and using a specific interface for the tool_calls array elements
instead of any[]. Add defensive checks to ensure tc.function exists before
accessing its properties to prevent runtime errors. Update the function to
handle cases where tc.function might be null or undefined, returning appropriate
defaults or skipping those entries safely.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/tools/searchGutenbergBooks.ts (2)
1-5: Consider improving type safety for the authors field.While
any[]works for a test tool, consider defining a more specific type for authors to improve type safety and code documentation.+export type Author = { + name: string; + birth_year?: number | null; + death_year?: number | null; +}; + export type Book = { id: number; title: string; - authors: any[]; + authors: Author[]; };
23-27: Consider improving type safety in the mapping function.While acceptable for a test tool, the
anytype could be more specific to improve type safety.- return data.results.map((book: any) => ({ + return data.results.map((book: { id: number; title: string; authors: any[] }) => ({
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
client/src/lib/db.ts(1 hunks)client/src/lib/message-normalise.ts(1 hunks)src/sync.ts(7 hunks)src/tools/index.ts(1 hunks)src/tools/searchGutenbergBooks.ts(1 hunks)src/tools/searchWeb.ts(1 hunks)src/tools/testSearch.ts(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- src/tools/index.ts
- client/src/lib/message-normalise.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- client/src/lib/db.ts
- src/sync.ts
🧰 Additional context used
🧠 Learnings (3)
src/tools/testSearch.ts (2)
Learnt from: Pablonara
PR: t35-turbo/TanTanChat#5
File: src/sync.ts:158-161
Timestamp: 2025-07-02T00:50:12.362Z
Learning: The searchGutenbergBooks and testSearch functions in src/sync.ts are placeholder tools for testing tool call functionality, not production code requiring robust error handling.
Learnt from: Pablonara
PR: t35-turbo/TanTanChat#5
File: src/sync.ts:83-94
Timestamp: 2025-07-02T00:29:04.108Z
Learning: The searchGutenbergBooks function in src/sync.ts is a placeholder tool for testing purposes, not production code requiring robust error handling.
src/tools/searchWeb.ts (2)
Learnt from: Pablonara
PR: t35-turbo/TanTanChat#5
File: src/sync.ts:158-161
Timestamp: 2025-07-02T00:50:12.362Z
Learning: The searchGutenbergBooks and testSearch functions in src/sync.ts are placeholder tools for testing tool call functionality, not production code requiring robust error handling.
Learnt from: Pablonara
PR: t35-turbo/TanTanChat#5
File: src/sync.ts:83-94
Timestamp: 2025-07-02T00:29:04.108Z
Learning: The searchGutenbergBooks function in src/sync.ts is a placeholder tool for testing purposes, not production code requiring robust error handling.
src/tools/searchGutenbergBooks.ts (2)
Learnt from: Pablonara
PR: t35-turbo/TanTanChat#5
File: src/sync.ts:158-161
Timestamp: 2025-07-02T00:50:12.362Z
Learning: The searchGutenbergBooks and testSearch functions in src/sync.ts are placeholder tools for testing tool call functionality, not production code requiring robust error handling.
Learnt from: Pablonara
PR: t35-turbo/TanTanChat#5
File: src/sync.ts:83-94
Timestamp: 2025-07-02T00:29:04.108Z
Learning: The searchGutenbergBooks function in src/sync.ts is a placeholder tool for testing purposes, not production code requiring robust error handling.
🧬 Code Graph Analysis (3)
src/tools/testSearch.ts (2)
src/tools/searchWeb.ts (1)
definition(42-89)src/tools/searchGutenbergBooks.ts (1)
definition(31-50)
src/tools/searchWeb.ts (2)
src/tools/testSearch.ts (1)
definition(13-24)src/tools/searchGutenbergBooks.ts (1)
definition(31-50)
src/tools/searchGutenbergBooks.ts (2)
src/tools/searchWeb.ts (1)
definition(42-89)src/tools/testSearch.ts (1)
definition(13-24)
🔇 Additional comments (7)
src/tools/testSearch.ts (3)
1-4: LGTM: Clear and concise documentationThe JSDoc comment effectively describes the function's purpose and return behavior.
5-10: LGTM: Well-structured test function implementationThe function implementation is clean and follows TypeScript best practices:
- Proper async/await pattern
- Explicit return type annotation
- Returns consistent data structure with message and timestamp
- Uses ISO string format for timestamp which is standardized
12-24: LGTM: Consistent with established tool definition patternThe OpenAI tool definition follows the same structure and TypeScript typing patterns as other tools in the codebase (e.g.,
searchGutenbergBooks,searchWeb):
- Uses
as constassertion for proper type inference- Function name matches the actual function implementation
- Empty parameters schema is appropriate for a parameterless test function
- Clear description that aligns with the function's purpose
src/tools/searchWeb.ts (2)
1-13: LGTM: Well-documented function interfaceThe JSDoc comments provide clear documentation for all parameters and return value, making the function easy to understand and use.
42-89: LGTM: Comprehensive and well-structured tool definitionThe OpenAI tool definition is excellent:
- Follows the established pattern from other tools in the codebase
- Provides detailed, helpful parameter descriptions with usage guidance
- Includes appropriate default values and required fields
- Parameter descriptions give clear guidance on when to use different options (e.g., "If performing image search set this to AT LEAST 5 or more")
src/tools/searchGutenbergBooks.ts (2)
17-20: LGTM: Appropriate error handling for test tool.The error handling is sufficient for a testing tool, properly checking response status and throwing descriptive errors.
31-50: LGTM: Tool definition follows established patterns.The OpenAI tool definition structure is consistent with other tools in the codebase (like
searchWeb.tsandtestSearch.ts), providing clear descriptions and proper parameter schemas.
| export default async function searchWeb( | ||
| query: string, | ||
| numResults: number = 3, | ||
| links: number = 1, | ||
| includeSubpages: number = 0, | ||
| fullPageText: boolean = false, | ||
| imageLinks: number = 0, | ||
| summary: boolean = false, | ||
| ): Promise<any> { | ||
| const exa = new Exa(process.env.EXASEARCH_API_KEY || ""); | ||
| const result = await exa.searchAndContents(query, { | ||
| context: true, | ||
| subpages: includeSubpages, | ||
| numResults, | ||
| extras: { | ||
| links, | ||
| imageLinks, | ||
| }, | ||
| summary: summary ? true : undefined, | ||
| text: fullPageText ? true : undefined, | ||
| }); | ||
|
|
||
| return result.context; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling and improve type safety
The function implementation has several areas for improvement:
- Missing error handling: No error handling for API failures or network issues
- Type safety: Return type
Promise<any>is too generic - API key validation: No validation that the API key exists
Consider applying these improvements:
-): Promise<any> {
+): Promise<unknown> {
const exa = new Exa(process.env.EXASEARCH_API_KEY || "");
+
+ if (!process.env.EXASEARCH_API_KEY) {
+ throw new Error("EXASEARCH_API_KEY environment variable is required");
+ }
+
+ try {
const result = await exa.searchAndContents(query, {
context: true,
subpages: includeSubpages,
numResults,
extras: {
links,
imageLinks,
},
summary: summary ? true : undefined,
text: fullPageText ? true : undefined,
});
return result.context;
+ } catch (error) {
+ console.error("Web search failed:", error);
+ throw new Error(`Web search failed: ${error instanceof Error ? error.message : "Unknown error"}`);
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export default async function searchWeb( | |
| query: string, | |
| numResults: number = 3, | |
| links: number = 1, | |
| includeSubpages: number = 0, | |
| fullPageText: boolean = false, | |
| imageLinks: number = 0, | |
| summary: boolean = false, | |
| ): Promise<any> { | |
| const exa = new Exa(process.env.EXASEARCH_API_KEY || ""); | |
| const result = await exa.searchAndContents(query, { | |
| context: true, | |
| subpages: includeSubpages, | |
| numResults, | |
| extras: { | |
| links, | |
| imageLinks, | |
| }, | |
| summary: summary ? true : undefined, | |
| text: fullPageText ? true : undefined, | |
| }); | |
| return result.context; | |
| } | |
| export default async function searchWeb( | |
| query: string, | |
| numResults: number = 3, | |
| links: number = 1, | |
| includeSubpages: number = 0, | |
| fullPageText: boolean = false, | |
| imageLinks: number = 0, | |
| summary: boolean = false, | |
| ): Promise<unknown> { | |
| const exa = new Exa(process.env.EXASEARCH_API_KEY || ""); | |
| if (!process.env.EXASEARCH_API_KEY) { | |
| throw new Error("EXASEARCH_API_KEY environment variable is required"); | |
| } | |
| try { | |
| const result = await exa.searchAndContents(query, { | |
| context: true, | |
| subpages: includeSubpages, | |
| numResults, | |
| extras: { | |
| links, | |
| imageLinks, | |
| }, | |
| summary: summary ? true : undefined, | |
| text: fullPageText ? true : undefined, | |
| }); | |
| return result.context; | |
| } catch (error) { | |
| console.error("Web search failed:", error); | |
| throw new Error(`Web search failed: ${error instanceof Error ? error.message : "Unknown error"}`); | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/tools/searchWeb.ts around lines 16 to 39, add error handling to catch and
manage potential API failures or network issues during the search call. Validate
that the API key exists before creating the Exa instance and throw a clear error
if it is missing. Replace the generic Promise<any> return type with a more
specific type that matches the expected structure of the search result context
to improve type safety.
| * @param searchTerms - An array of keywords to search for (e.g. ['dickens', 'great']). | ||
| * @returns A list of matching books with their basic metadata. | ||
| */ | ||
| export default async function searchGutenbergBooks(searchTerms: string[]): Promise<Book[]> { |
There was a problem hiding this comment.
Critical: Parameter name mismatch between function and tool definition.
The function expects searchTerms but the OpenAI tool definition uses search_terms (line 40). This mismatch will cause the tool system to fail when invoking the function.
-export default async function searchGutenbergBooks(searchTerms: string[]): Promise<Book[]> {
+export default async function searchGutenbergBooks(search_terms: string[]): Promise<Book[]> {And update the function body accordingly:
- const searchQuery = searchTerms.join(" ");
+ const searchQuery = search_terms.join(" ");Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/tools/searchGutenbergBooks.ts at line 13, the function parameter is named
searchTerms but the OpenAI tool definition uses search_terms. Rename the
function parameter from searchTerms to search_terms to match the tool definition
and update all references to this parameter within the function body accordingly
to ensure consistency and proper invocation.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.cursorrules(1 hunks)client/src/components/MessageRenderer.tsx(7 hunks)client/src/lib/db.ts(1 hunks)src/sync.ts(7 hunks)src/tools/index.ts(1 hunks)src/tools/searchWeb.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- .cursorrules
🚧 Files skipped from review as they are similar to previous changes (3)
- client/src/lib/db.ts
- src/tools/searchWeb.ts
- client/src/components/MessageRenderer.tsx
🧰 Additional context used
🧠 Learnings (2)
src/tools/index.ts (3)
Learnt from: Pablonara
PR: t35-turbo/TanTanChat#5
File: src/sync.ts:324-338
Timestamp: 2025-07-02T01:04:58.995Z
Learning: In src/sync.ts, the tool call accumulation logic for toolCallChunk.index doesn't require additional validation because upstream components validate tool call data before it reaches the accumulation logic, preventing invalid indices from getting to this point.
Learnt from: Pablonara
PR: t35-turbo/TanTanChat#5
File: src/sync.ts:158-161
Timestamp: 2025-07-02T00:50:12.362Z
Learning: The searchGutenbergBooks and testSearch functions in src/sync.ts are placeholder tools for testing tool call functionality, not production code requiring robust error handling.
Learnt from: Pablonara
PR: t35-turbo/TanTanChat#5
File: src/sync.ts:83-94
Timestamp: 2025-07-02T00:29:04.108Z
Learning: The searchGutenbergBooks function in src/sync.ts is a placeholder tool for testing purposes, not production code requiring robust error handling.
src/sync.ts (4)
<retrieved_learning>
Learnt from: Pablonara
PR: #5
File: src/sync.ts:324-338
Timestamp: 2025-07-02T01:04:58.995Z
Learning: In src/sync.ts, the tool call accumulation logic for toolCallChunk.index doesn't require additional validation because upstream components validate tool call data before it reaches the accumulation logic, preventing invalid indices from getting to this point.
</retrieved_learning>
<retrieved_learning>
Learnt from: Pablonara
PR: #5
File: src/sync.ts:83-94
Timestamp: 2025-07-02T00:29:04.108Z
Learning: The searchGutenbergBooks function in src/sync.ts is a placeholder tool for testing purposes, not production code requiring robust error handling.
</retrieved_learning>
<retrieved_learning>
Learnt from: Pablonara
PR: #5
File: src/sync.ts:158-161
Timestamp: 2025-07-02T00:50:12.362Z
Learning: The searchGutenbergBooks and testSearch functions in src/sync.ts are placeholder tools for testing tool call functionality, not production code requiring robust error handling.
</retrieved_learning>
<retrieved_learning>
Learnt from: Pablonara
PR: #5
File: client/src/components/MessageRenderer.tsx:0-0
Timestamp: 2025-07-01T22:16:48.218Z
Learning: In TanTanChat MessageRenderer component, user messages do not generate reasoning output, so reasoning display logic should only exist for assistant/system/tool messages, not user messages.
</retrieved_learning>
🔇 Additional comments (12)
src/tools/index.ts (3)
1-1: LGTM: Clean module import pattern.The wildcard import pattern works well for organizing tool modules and their exports.
4-6: LGTM: Well-structured tool definitions export.The tools array provides a clean interface for OpenAI tool definitions and can easily accommodate additional tools.
9-11: LGTM: Good use of const assertion for type safety.The
as constassertion ensures proper type inference for the tool mapping object.src/sync.ts (9)
5-5: LGTM! Clean imports and type extensions.The new imports properly support the tool call functionality, and the Messages type extensions with optional fields maintain backward compatibility while enabling tool call support.
Also applies to: 10-11, 15-15, 20-21
74-74: Approve recursion control implementation.The depth parameter threading and recursion warning provide reasonable temporary protection against infinite tool call loops. The implementation correctly passes depth through the call chain.
Also applies to: 77-77, 83-83, 86-88
91-92: LGTM! Simple and effective accumulator initialization.The tool call accumulator is properly initialized for the streaming accumulation pattern.
161-161: Correct fix for system prompt depth parameter.The depth parameter is now properly passed to
default_prompt(), ensuring the prompt engineering recursion control functions as intended.
183-212: Excellent message mapping implementation.The role-specific message handling correctly maps tool messages with call IDs and properly serializes assistant tool calls. The conditional inclusion of tool_calls only when present prevents unnecessary empty arrays.
223-224: Good debugging and API integration.The debug logging provides visibility into message structure, and the tools array properly enables function calling in the OpenAI API.
Also applies to: 227-227
244-267: Solid tool call accumulation logic.The streaming tool call accumulation correctly handles chunk-by-chunk building of tool calls, with proper initialization and argument concatenation. The index-based approach aligns well with the OpenAI streaming protocol.
277-357: Comprehensive tool execution and continuation logic.The implementation correctly:
- Executes tool calls when detected
- Stores tool results as proper database entries
- Efficiently queries for specific tool messages
- Constructs complete message context for conversation continuation
- Properly increments recursion depth
This provides a complete tool call execution cycle.
447-490: Robust tool call parsing with error handling.The implementation properly:
- Extracts tool calls from stream chunks
- Handles JSON parsing errors gracefully
- Converts string arguments back to objects for database storage
- Provides debug logging for troubleshooting
The error handling ensures the system continues operating even with malformed tool call data.
| export async function executeTool(toolName: string, args: any): Promise<any> { | ||
| switch (toolName) { | ||
| case "searchWeb": | ||
| return TOOL_MAPPING.searchWeb(args); | ||
| default: | ||
| throw new Error(`Unknown tool function: ${toolName}`); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor to eliminate code duplication and improve type safety.
The executeTool function duplicates the mapping logic instead of leveraging the existing TOOL_MAPPING object, and uses any types which reduces type safety.
Apply this diff to eliminate duplication and improve type safety:
-export async function executeTool(toolName: string, args: any): Promise<any> {
- switch (toolName) {
- case "searchWeb":
- return TOOL_MAPPING.searchWeb(args);
- default:
- throw new Error(`Unknown tool function: ${toolName}`);
- }
-}
+export async function executeTool(
+ toolName: keyof typeof TOOL_MAPPING,
+ args: any
+): Promise<any> {
+ const toolFunction = TOOL_MAPPING[toolName];
+ if (!toolFunction) {
+ throw new Error(`Unknown tool function: ${toolName}`);
+ }
+ return toolFunction(args);
+}This approach:
- Eliminates the need to maintain both TOOL_MAPPING and switch cases
- Uses the mapping object directly for better maintainability
- Provides better type safety with
keyof typeof TOOL_MAPPING - Automatically supports new tools added to TOOL_MAPPING
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export async function executeTool(toolName: string, args: any): Promise<any> { | |
| switch (toolName) { | |
| case "searchWeb": | |
| return TOOL_MAPPING.searchWeb(args); | |
| default: | |
| throw new Error(`Unknown tool function: ${toolName}`); | |
| } | |
| } | |
| export async function executeTool( | |
| toolName: keyof typeof TOOL_MAPPING, | |
| args: any | |
| ): Promise<any> { | |
| const toolFunction = TOOL_MAPPING[toolName]; | |
| if (!toolFunction) { | |
| throw new Error(`Unknown tool function: ${toolName}`); | |
| } | |
| return toolFunction(args); | |
| } |
🤖 Prompt for AI Agents
In src/tools/index.ts around lines 14 to 21, the executeTool function duplicates
the tool mapping logic and uses any types, reducing type safety. Refactor
executeTool to accept toolName as keyof typeof TOOL_MAPPING and directly invoke
TOOL_MAPPING[toolName] with args, removing the switch statement. This eliminates
duplication, improves maintainability, and enhances type safety by leveraging
the existing TOOL_MAPPING object.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
client/src/components/MessageRenderer.tsx (1)
299-393: Consider extracting complex logic into utility functionsThe message rendering logic is comprehensive but has some areas that could benefit from refactoring:
JSON parsing complexity (lines 339-351): The inline JSON parsing and formatting logic is complex and could be extracted into a utility function for better maintainability and testing.
Deep nesting: The conditional rendering structure is quite deep, which impacts readability.
Consider extracting the JSON formatting logic:
// In a separate utility file function formatToolResult(message: string): string { try { const parsed = JSON.parse(message); return typeof parsed === 'string' ? parsed.replace(/\\n/g, '\n') : JSON.stringify(parsed, null, 2); } catch { return message.replace(/\\n/g, '\n'); } }This would simplify the JSX and make the logic more testable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
client/src/components/MessageRenderer.tsx(7 hunks)
🧰 Additional context used
🧠 Learnings (1)
client/src/components/MessageRenderer.tsx (1)
Learnt from: Pablonara
PR: t35-turbo/TanTanChat#5
File: client/src/components/MessageRenderer.tsx:0-0
Timestamp: 2025-07-01T22:16:48.218Z
Learning: In TanTanChat MessageRenderer component, user messages do not generate reasoning output, so reasoning display logic should only exist for assistant/system/tool messages, not user messages.
🔇 Additional comments (5)
client/src/components/MessageRenderer.tsx (5)
28-28: LGTM: Import statement for message normalization utilitiesThe import of
normaliseToolCallsandsafeRoleutilities is appropriate for the tool call support functionality added to this component.
68-90: Excellent improvement to message fetching with proper error handlingThe new implementation addresses previous type safety and error handling concerns:
- Uses
safeParse()instead ofparse()for safer validation- Proper error handling that returns empty messages array on parse failure
- Message normalization ensures consistent tool call formatting
- Cleaner return structure
This is a significant improvement over the previous implementation.
114-115: LGTM: Tool call fields added for type consistencyAdding
tool_callsandtoolCallIdfields to pending user messages and active assistant messages maintains consistency with theMessagetype definition. Setting them tonullis appropriate for these message types.Also applies to: 131-132
173-174: LGTM: State variables for tool call UI controlsThe new state variables
showToolResultandshowFunctionCallsfollow the established pattern and are appropriately named for controlling the visibility of tool-related collapsible sections.
395-395: LGTM: Improved alert condition for tool call finish reasonsExcluding
"tool_calls"and"tool_calls_response"from the alert display is appropriate, as these are normal operational states during tool execution rather than error conditions that warrant user alerts.
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
client/src/lib/utils.ts (1)
10-30: Consider deprecating these utility functions in favor of the useKeyToasts hook.These functions call hooks inside utility functions, which violates the Rules of Hooks. The new
useKeyToastshook provides a better solution by calling hooks at the top level.Consider marking these functions as deprecated and migrating all usage to
useKeyToasts:+/** + * @deprecated Use useKeyToasts hook instead to avoid Rules of Hooks violations + */ export function toastEnterAPIKey(reason: 'missing' | 'invalid' = 'missing') {
🧹 Nitpick comments (2)
client/src/components/KeyInputModal.tsx (2)
69-101: Consider refactoring to reduce code duplication.The
ExaKeyInputModalcomponent duplicates significant logic from the mainKeyInputModal. While the current approach works, consider extracting a reusableBaseKeyInputModalcomponent to reduce duplication.// Example refactor approach: +function BaseKeyInputModal({ + isOpen, + toggle, + setKey, + title, + placeholder, + additionalContent +}: BaseKeyInputModalProps) { + const [key, setKeyLocal] = useState(""); + + return ( + <Dialog open={isOpen} onOpenChange={toggle}> + <DialogContent> + <DialogHeader> + <DialogTitle>{title}</DialogTitle> + <span>Your key is stored locally and is never saved on our server.</span> + </DialogHeader> + {additionalContent} + <form onSubmit={handleSubmit} className="flex space-x-2"> + <Input placeholder={placeholder} value={key} onChange={(e) => setKeyLocal(e.target.value)} /> + <Button type="submit">Save</Button> + </form> + </DialogContent> + </Dialog> + ); +}
84-88: Minor: Consider consistent form submission order.The form submission order differs between the two modals. In the main modal,
setKey(key)is called beforee.preventDefault(), while in the Exa modal, it's called after. Consider using a consistent order for maintainability.- e.preventDefault(); - setKey(key); + setKey(key); + e.preventDefault();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
client/package-lock.jsonis excluded by!**/package-lock.jsonclient/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (17)
.cursorrules(1 hunks)client/package.json(1 hunks)client/src/components/KeyInputModal.tsx(2 hunks)client/src/components/ModelSelector.tsx(3 hunks)client/src/components/Onboarding.tsx(1 hunks)client/src/components/ui/switch.tsx(1 hunks)client/src/hooks/use-exa-key.ts(1 hunks)client/src/hooks/use-key-input.ts(1 hunks)client/src/hooks/use-key-toasts.ts(1 hunks)client/src/hooks/use-tools.ts(1 hunks)client/src/lib/utils.ts(3 hunks)client/src/routes/__root.tsx(2 hunks)client/src/routes/chat/$chatId.tsx(4 hunks)client/src/routes/or_auth.tsx(1 hunks)client/src/routes/settings.tsx(1 hunks)client/src/styles.css(1 hunks)src/chats.ts(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- client/package.json
- client/src/styles.css
- client/src/components/ui/switch.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- src/chats.ts
- .cursorrules
🧰 Additional context used
🧠 Learnings (2)
client/src/hooks/use-tools.ts (1)
Learnt from: Pablonara
PR: t35-turbo/TanTanChat#5
File: src/sync.ts:158-161
Timestamp: 2025-07-02T00:50:12.362Z
Learning: The searchGutenbergBooks and testSearch functions in src/sync.ts are placeholder tools for testing tool call functionality, not production code requiring robust error handling.
client/src/components/ModelSelector.tsx (1)
Learnt from: Pablonara
PR: t35-turbo/TanTanChat#5
File: src/sync.ts:158-161
Timestamp: 2025-07-02T00:50:12.362Z
Learning: The searchGutenbergBooks and testSearch functions in src/sync.ts are placeholder tools for testing tool call functionality, not production code requiring robust error handling.
🧬 Code Graph Analysis (9)
client/src/routes/__root.tsx (1)
client/src/components/KeyInputModal.tsx (1)
ExaKeyInputModal(69-101)
client/src/routes/or_auth.tsx (1)
client/src/hooks/use-key-input.ts (1)
useKeyInput(14-29)
client/src/routes/settings.tsx (1)
client/src/hooks/use-key-input.ts (1)
useKeyInput(14-29)
client/src/components/Onboarding.tsx (1)
client/src/hooks/use-key-input.ts (1)
useKeyInput(14-29)
client/src/routes/chat/$chatId.tsx (2)
client/src/hooks/use-exa-key.ts (1)
useExaKey(10-20)client/src/hooks/use-key-toasts.ts (1)
useKeyToasts(10-45)
client/src/hooks/use-key-toasts.ts (2)
client/src/hooks/use-key-input.ts (1)
useKeyInput(14-29)client/src/lib/utils.ts (2)
toastEnterAPIKey(10-30)toastEnterExaAPIKey(32-41)
client/src/lib/utils.ts (1)
client/src/hooks/use-key-input.ts (1)
useKeyInput(14-29)
client/src/components/KeyInputModal.tsx (2)
client/src/hooks/use-key-input.ts (1)
useKeyInput(14-29)client/src/hooks/use-exa-key.ts (1)
useExaKey(10-20)
client/src/components/ModelSelector.tsx (3)
client/src/hooks/use-key-toasts.ts (1)
useKeyToasts(10-45)client/src/hooks/use-tools.ts (1)
useTools(9-19)client/src/hooks/use-exa-key.ts (1)
useExaKey(10-20)
🔇 Additional comments (23)
client/src/hooks/use-exa-key.ts (1)
1-20: LGTM! Well-implemented persistent API key management.The implementation follows established patterns and correctly uses Zustand's persist middleware. The localStorage storage is appropriate for client-side API key management, aligning with the application's privacy approach.
client/src/hooks/use-tools.ts (2)
2-2: Good addition of persistence for improved UX.Adding the persist middleware import enables tool preferences to be maintained across sessions.
9-19: Correct implementation of persistent tools state.The state structure remains unchanged while adding persistence functionality. The "tools-state" key is appropriately descriptive for localStorage identification.
client/src/routes/__root.tsx (2)
2-2: Proper import of the new Exa key modal component.The ExaKeyInputModal is correctly imported alongside the existing KeyInputModal.
15-15: Correct integration of ExaKeyInputModal in the root component.The modal is properly placed within the QueryClientProvider context alongside the existing KeyInputModal, ensuring global availability throughout the application.
client/src/routes/or_auth.tsx (1)
15-15: Correct state selector update for refactored key input hook.The change from
state.opentostate.openAIproperly aligns with the refactoreduseKeyInputhook that now manages separate states for AI and Exa API key modals.client/src/components/Onboarding.tsx (1)
8-8: Consistent state selector update for refactored hook.The change from
state.opentostate.openAIis consistent with the refactoreduseKeyInputhook pattern and maintains proper functionality for opening the AI key input modal.client/src/routes/settings.tsx (1)
88-88: LGTM! Correctly updated selector for refactored hook.The change from
state.opentostate.openAIproperly aligns with the refactoreduseKeyInputhook that now manages separate modal states for AI and Exa keys.client/src/routes/chat/$chatId.tsx (3)
24-24: LGTM! Proper integration of Exa key management.The addition of
useExaKeyhook and its usage is consistent with the dual API key architecture.Also applies to: 37-37
19-19: LGTM! Improved hook-based toast architecture.The replacement of direct
toastEnterAPIKeyimport with theuseKeyToastshook follows React's Rules of Hooks and provides better encapsulation of modal state management.Also applies to: 123-123
78-78: Confirm inclusion ofexa_keyin follow-up message requestsA repo-wide grep for
exaKey/exa_keyonly shows it being passed during new chat creation (line 78). The regular message-send block (lines 95–105) doesn’t includeexa_key. Please verify with the backend team whether omittingexa_keyon subsequent messages is intentional or if it needs to be added.• client/src/routes/chat/$chatId.tsx
– line 78: new chat creation options includeexaKey: exa_key
– lines 95–105: POST body for sending messages omitsexa_keyclient/src/hooks/use-key-toasts.ts (1)
1-45: Excellent architectural improvement!This hook elegantly solves the Rules of Hooks violation that would occur when calling
useKeyInputinside event handlers or utility functions. The implementation is clean, well-documented, and follows React best practices.client/src/hooks/use-key-input.ts (2)
3-12: LGTM! Clean and consistent type definitions.The type definitions properly reflect the dual modal state architecture with consistent naming patterns for both AI and Exa modal controls.
14-29: LGTM! Well-implemented dual modal state management.The implementation maintains consistency between AI and Exa modal controls, properly handles toggle functionality with both explicit boolean values and state toggling, and follows established patterns.
client/src/components/KeyInputModal.tsx (2)
8-8: LGTM: Import addition is appropriate.The import of
useExaKeyhook aligns with the PR objective of adding tool call support, specifically for Exa Search API key management.
11-12: LGTM: State selector renaming maintains clarity.Renaming
isOpen/toggletoisAIOpen/toggleAIproperly distinguishes between the AI key modal and the new Exa key modal states, preventing confusion.client/src/components/ModelSelector.tsx (7)
4-4: LGTM: Icon imports support the new tool functionality.The additional icon imports (
ChevronUp,Globe,PocketKnife) are used appropriately in the newToolSelectorcomponent.
9-9: LGTM: Hook imports align with tool management architecture.The imports for
useKeyToasts,Switch,useTools,Tooltip, anduseExaKeyproperly support the new tool configuration functionality.Also applies to: 19-22
172-172: LGTM: Consistent use of toast hook.Refactoring to use the
useKeyToastshook instead of direct import maintains consistency with the centralized toast management approach.
270-270: LGTM: Improved accessibility with cursor-pointer.Adding
cursor-pointerclass to file input elements improves user experience by providing visual feedback that the elements are interactive.Also applies to: 278-278
292-331: LGTM: Well-structured tool selector with proper validation.The
ToolSelectorcomponent implements appropriate validation logic:
- Checks for Exa API key before enabling web search
- Provides clear user feedback through toasts
- Uses proper UI components for accessibility
- Follows consistent patterns with other components in the codebase
The integration with the key management system ensures users can't enable tools without proper configuration.
300-308: LGTM: Robust validation prevents misconfiguration.The
handleWebSearchTogglefunction properly validates that the Exa API key is present before enabling web search functionality. The early return pattern prevents the toggle from being enabled without the required key, which is excellent UX.
285-285: LGTM: Clean integration of tool selector.The
ToolSelectorcomponent is properly integrated into the main component layout, maintaining the existing structure while adding new functionality.
| export function toastEnterExaAPIKey() { | ||
| const openModal = useKeyInput(state => state.openExa); | ||
|
|
||
| toast.error("Please set your Exa Search API Key first.", { | ||
| action: { | ||
| label: "Enter Key", | ||
| onClick: openModal, | ||
| }, | ||
| }); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Same Rules of Hooks concern applies to the new function.
The new toastEnterExaAPIKey function has the same hook usage issue as the existing toastEnterAPIKey function.
Consider marking this function as deprecated as well:
+/**
+ * @deprecated Use useKeyToasts hook instead to avoid Rules of Hooks violations
+ */
export function toastEnterExaAPIKey() {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export function toastEnterExaAPIKey() { | |
| const openModal = useKeyInput(state => state.openExa); | |
| toast.error("Please set your Exa Search API Key first.", { | |
| action: { | |
| label: "Enter Key", | |
| onClick: openModal, | |
| }, | |
| }); | |
| } | |
| /** | |
| * @deprecated Use useKeyToasts hook instead to avoid Rules of Hooks violations | |
| */ | |
| export function toastEnterExaAPIKey() { | |
| const openModal = useKeyInput(state => state.openExa); | |
| toast.error("Please set your Exa Search API Key first.", { | |
| action: { | |
| label: "Enter Key", | |
| onClick: openModal, | |
| }, | |
| }); | |
| } |
🤖 Prompt for AI Agents
In client/src/lib/utils.ts around lines 32 to 41, the function
toastEnterExaAPIKey uses the React hook useKeyInput inside a non-component
function, violating the Rules of Hooks. To fix this, avoid calling the hook
directly in this function; instead, refactor to pass the openModal callback as
an argument or move the hook call to a component context. Additionally, mark
this function as deprecated to indicate it should no longer be used.
- Replace .mov files with .mp4 versions for GitHub compatibility - Add showcase videos section to README.md with links to converted files - Remove original .mov files to reduce repository size 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
…development scenarios (starting a postgres container will not require 30s)
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
src/tools/index.ts (1)
14-23: Refactor to eliminate code duplication and improve type safety.The
executeToolfunction still duplicates the mapping logic instead of leveraging the existingTOOL_MAPPINGobject, and usesanytypes which reduces type safety.Apply this diff to eliminate duplication and improve type safety:
-export async function executeTool(toolName: string, args: any): Promise<any> { - switch (toolName) { - case "searchWeb": - return TOOL_MAPPING.searchWeb(args); - case "getSiteContents": - return TOOL_MAPPING.getSiteContents(args); - default: - throw new Error(`Unknown tool function: ${toolName}`); - } -} +export async function executeTool( + toolName: keyof typeof TOOL_MAPPING, + args: any +): Promise<any> { + const toolFunction = TOOL_MAPPING[toolName]; + if (!toolFunction) { + throw new Error(`Unknown tool function: ${toolName}`); + } + return toolFunction(args); +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.env.example(1 hunks)src/db/schema.ts(4 hunks)src/lib/sys_prompts.ts(4 hunks)src/tools/getSiteContents.ts(1 hunks)src/tools/index.ts(1 hunks)src/tools/searchWeb.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- .env.example
🚧 Files skipped from review as they are similar to previous changes (2)
- src/tools/searchWeb.ts
- src/db/schema.ts
🧰 Additional context used
🧠 Learnings (2)
src/tools/index.ts (3)
Learnt from: Pablonara
PR: t35-turbo/TanTanChat#5
File: src/sync.ts:158-161
Timestamp: 2025-07-02T00:50:12.375Z
Learning: The searchGutenbergBooks and testSearch functions in src/sync.ts are placeholder tools for testing tool call functionality, not production code requiring robust error handling.
Learnt from: Pablonara
PR: t35-turbo/TanTanChat#5
File: src/sync.ts:324-338
Timestamp: 2025-07-02T01:04:59.021Z
Learning: In src/sync.ts, the tool call accumulation logic for toolCallChunk.index doesn't require additional validation because upstream components validate tool call data before it reaches the accumulation logic, preventing invalid indices from getting to this point.
Learnt from: Pablonara
PR: t35-turbo/TanTanChat#5
File: src/sync.ts:83-94
Timestamp: 2025-07-02T00:29:04.138Z
Learning: The searchGutenbergBooks function in src/sync.ts is a placeholder tool for testing purposes, not production code requiring robust error handling.
src/tools/getSiteContents.ts (1)
Learnt from: Pablonara
PR: t35-turbo/TanTanChat#5
File: src/sync.ts:158-161
Timestamp: 2025-07-02T00:50:12.375Z
Learning: The searchGutenbergBooks and testSearch functions in src/sync.ts are placeholder tools for testing tool call functionality, not production code requiring robust error handling.
🔇 Additional comments (3)
src/lib/sys_prompts.ts (3)
1-1: Function signature update looks good.The addition of the
toolCallDepthparameter properly integrates with the tool call tracking system and enables recursion depth management.
34-53: Comprehensive tool usage guidance enhances AI behavior.The detailed instructions for
searchWebandgetSiteContentsprovide clear guidance on best practices, pagination, and avoiding common pitfalls. This will help ensure consistent and effective tool usage.
239-242: Effective recursion depth management implementation.The tool call depth tracking and user notification system provides a good safety mechanism to prevent infinite tool call loops while giving users control over the process.
| import { db } from "../db"; | ||
| import { pageContents } from "../db/schema"; | ||
| import { eq, gte } from "drizzle-orm"; | ||
| const REFRESH_PAGE_TIME_DIFF = parseInt(process.env.REFRESH_PAGE_TIME_DIFF ?? "300", 10); |
There was a problem hiding this comment.
Fix unit mismatch in cache refresh timing.
The REFRESH_PAGE_TIME_DIFF variable is parsed as a plain number (defaulting to 300, which suggests seconds) but is used directly as milliseconds in the cache freshness calculation, causing the cache to refresh every 300ms instead of 5 minutes.
Apply this diff to fix the unit conversion:
-const REFRESH_PAGE_TIME_DIFF = parseInt(process.env.REFRESH_PAGE_TIME_DIFF ?? "300", 10);
+const REFRESH_PAGE_TIME_DIFF = parseInt(process.env.REFRESH_PAGE_TIME_DIFF ?? "300", 10) * 1000;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const REFRESH_PAGE_TIME_DIFF = parseInt(process.env.REFRESH_PAGE_TIME_DIFF ?? "300", 10); | |
| const REFRESH_PAGE_TIME_DIFF = parseInt(process.env.REFRESH_PAGE_TIME_DIFF ?? "300", 10) * 1000; |
🤖 Prompt for AI Agents
In src/tools/getSiteContents.ts at line 20, the REFRESH_PAGE_TIME_DIFF is parsed
as seconds but used as milliseconds, causing incorrect cache refresh timing. Fix
this by multiplying the parsed value by 1000 to convert seconds to milliseconds,
ensuring the cache refresh interval is correctly set to 5 minutes by default.
| export async function toolFunction(rawArgs: getSiteContentsQueryArgs): Promise<any> { | ||
| // Validate and enrich args with defaults | ||
| const { url, links, includeSubpages, imageLinks, offset, numLines } = searchWebArgsSchema.parse(rawArgs); | ||
| let pageContents = await checkCachedPageContents(url); | ||
| let totalLength = 0; | ||
| if (pageContents !== null) { | ||
| totalLength = pageContents.contents.split("\n").length; | ||
| return "Total length: " + totalLength + " Contents: " + pageContents; | ||
| } else { | ||
| const exa = new Exa(process.env.EXASEARCH_API_KEY || ""); | ||
| const result = await exa.getContents([url], { | ||
| text: true, | ||
| context: true, | ||
| extras: { | ||
| links: links, | ||
| imageLinks: imageLinks, | ||
| }, | ||
| }); | ||
| await updateCachedPageContents(url, result.context ?? ""); | ||
| // Handle offset and numLines | ||
| let finalContents = result.context ?? ""; | ||
| let totalLength = finalContents.split("\n").length; | ||
| console.log(result.statuses); | ||
| if (offset > 0 || numLines < Infinity) { | ||
| const lines = finalContents.split("\n"); | ||
| const startLine = Math.min(offset, lines.length); | ||
| const endLine = Math.min(startLine + numLines, lines.length); | ||
| finalContents = lines.slice(startLine, endLine).join("\n"); | ||
| } | ||
| if (totalLength > finalContents.split("\n").length) { | ||
| return ( | ||
| "Total length: " + | ||
| totalLength + | ||
| ` (Tip): The following text content is truncated due to size (content includes lines ${offset} to lines ${offset + numLines}). Use an offset and the numLines parameter to view more. \n` + | ||
| "Contents: " + | ||
| finalContents | ||
| ); | ||
| } else { | ||
| return "Total length: " + totalLength + " Contents: " + finalContents; | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for external API calls.
The Exa API call lacks proper error handling, which could cause the tool to fail unexpectedly for network issues, API errors, or invalid URLs.
Consider wrapping the Exa API call in try-catch:
+ try {
const exa = new Exa(process.env.EXASEARCH_API_KEY || "");
const result = await exa.getContents([url], {
text: true,
context: true,
extras: {
links: links,
imageLinks: imageLinks,
},
});
+ } catch (error) {
+ return `Error retrieving content from ${url}: ${error instanceof Error ? error.message : 'Unknown error'}`;
+ }Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/tools/getSiteContents.ts between lines 92 and 132, the call to the Exa
API lacks error handling, which may cause the function to fail unexpectedly on
network or API errors. Wrap the Exa API call and subsequent processing in a
try-catch block to catch any exceptions. In the catch block, handle errors
gracefully by logging the error and returning a meaningful error message or
fallback value to prevent the tool from crashing.
| export async function toolFunction(rawArgs: getSiteContentsQueryArgs): Promise<any> { | ||
| // Validate and enrich args with defaults | ||
| const { url, links, includeSubpages, imageLinks, offset, numLines } = searchWebArgsSchema.parse(rawArgs); | ||
| let pageContents = await checkCachedPageContents(url); | ||
| let totalLength = 0; | ||
| if (pageContents !== null) { | ||
| totalLength = pageContents.contents.split("\n").length; | ||
| return "Total length: " + totalLength + " Contents: " + pageContents; | ||
| } else { | ||
| const exa = new Exa(process.env.EXASEARCH_API_KEY || ""); | ||
| const result = await exa.getContents([url], { | ||
| text: true, | ||
| context: true, | ||
| extras: { | ||
| links: links, | ||
| imageLinks: imageLinks, | ||
| }, | ||
| }); | ||
| await updateCachedPageContents(url, result.context ?? ""); | ||
| // Handle offset and numLines | ||
| let finalContents = result.context ?? ""; | ||
| let totalLength = finalContents.split("\n").length; | ||
| console.log(result.statuses); | ||
| if (offset > 0 || numLines < Infinity) { | ||
| const lines = finalContents.split("\n"); | ||
| const startLine = Math.min(offset, lines.length); | ||
| const endLine = Math.min(startLine + numLines, lines.length); | ||
| finalContents = lines.slice(startLine, endLine).join("\n"); | ||
| } | ||
| if (totalLength > finalContents.split("\n").length) { | ||
| return ( | ||
| "Total length: " + | ||
| totalLength + | ||
| ` (Tip): The following text content is truncated due to size (content includes lines ${offset} to lines ${offset + numLines}). Use an offset and the numLines parameter to view more. \n` + | ||
| "Contents: " + | ||
| finalContents | ||
| ); | ||
| } else { | ||
| return "Total length: " + totalLength + " Contents: " + finalContents; | ||
| } | ||
| } | ||
| //const exa = new Exa(process.env.EXASEARCH_API_KEY || ""); | ||
|
|
||
| //return result.context; | ||
| } |
There was a problem hiding this comment.
Fix several issues in the toolFunction implementation.
The function has multiple issues that need to be addressed:
- Return statement bug (line 99): Returns object instead of string content
- Debug logging (line 114): Console.log should be removed or replaced with proper logging
- Dead code (lines 133-135): Commented code should be removed
- Variable naming confusion:
pageContentsvariable shadows the imported table name
Apply this diff to fix these issues:
export async function toolFunction(rawArgs: getSiteContentsQueryArgs): Promise<any> {
// Validate and enrich args with defaults
const { url, links, includeSubpages, imageLinks, offset, numLines } = searchWebArgsSchema.parse(rawArgs);
- let pageContents = await checkCachedPageContents(url);
+ let cachedPage = await checkCachedPageContents(url);
let totalLength = 0;
- if (pageContents !== null) {
- totalLength = pageContents.contents.split("\n").length;
- return "Total length: " + totalLength + " Contents: " + pageContents;
+ if (cachedPage !== null) {
+ totalLength = cachedPage.contents.split("\n").length;
+ return "Total length: " + totalLength + " Contents: " + cachedPage.contents;
} else {
const exa = new Exa(process.env.EXASEARCH_API_KEY || "");
const result = await exa.getContents([url], {
text: true,
context: true,
extras: {
links: links,
imageLinks: imageLinks,
},
});
await updateCachedPageContents(url, result.context ?? "");
// Handle offset and numLines
let finalContents = result.context ?? "";
let totalLength = finalContents.split("\n").length;
- console.log(result.statuses);
if (offset > 0 || numLines < Infinity) {
const lines = finalContents.split("\n");
const startLine = Math.min(offset, lines.length);
const endLine = Math.min(startLine + numLines, lines.length);
finalContents = lines.slice(startLine, endLine).join("\n");
}
if (totalLength > finalContents.split("\n").length) {
return (
"Total length: " +
totalLength +
` (Tip): The following text content is truncated due to size (content includes lines ${offset} to lines ${offset + numLines}). Use an offset and the numLines parameter to view more. \n` +
"Contents: " +
finalContents
);
} else {
return "Total length: " + totalLength + " Contents: " + finalContents;
}
}
- //const exa = new Exa(process.env.EXASEARCH_API_KEY || "");
-
- //return result.context;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export async function toolFunction(rawArgs: getSiteContentsQueryArgs): Promise<any> { | |
| // Validate and enrich args with defaults | |
| const { url, links, includeSubpages, imageLinks, offset, numLines } = searchWebArgsSchema.parse(rawArgs); | |
| let pageContents = await checkCachedPageContents(url); | |
| let totalLength = 0; | |
| if (pageContents !== null) { | |
| totalLength = pageContents.contents.split("\n").length; | |
| return "Total length: " + totalLength + " Contents: " + pageContents; | |
| } else { | |
| const exa = new Exa(process.env.EXASEARCH_API_KEY || ""); | |
| const result = await exa.getContents([url], { | |
| text: true, | |
| context: true, | |
| extras: { | |
| links: links, | |
| imageLinks: imageLinks, | |
| }, | |
| }); | |
| await updateCachedPageContents(url, result.context ?? ""); | |
| // Handle offset and numLines | |
| let finalContents = result.context ?? ""; | |
| let totalLength = finalContents.split("\n").length; | |
| console.log(result.statuses); | |
| if (offset > 0 || numLines < Infinity) { | |
| const lines = finalContents.split("\n"); | |
| const startLine = Math.min(offset, lines.length); | |
| const endLine = Math.min(startLine + numLines, lines.length); | |
| finalContents = lines.slice(startLine, endLine).join("\n"); | |
| } | |
| if (totalLength > finalContents.split("\n").length) { | |
| return ( | |
| "Total length: " + | |
| totalLength + | |
| ` (Tip): The following text content is truncated due to size (content includes lines ${offset} to lines ${offset + numLines}). Use an offset and the numLines parameter to view more. \n` + | |
| "Contents: " + | |
| finalContents | |
| ); | |
| } else { | |
| return "Total length: " + totalLength + " Contents: " + finalContents; | |
| } | |
| } | |
| //const exa = new Exa(process.env.EXASEARCH_API_KEY || ""); | |
| //return result.context; | |
| } | |
| export async function toolFunction(rawArgs: getSiteContentsQueryArgs): Promise<any> { | |
| // Validate and enrich args with defaults | |
| const { url, links, includeSubpages, imageLinks, offset, numLines } = searchWebArgsSchema.parse(rawArgs); | |
| let cachedPage = await checkCachedPageContents(url); | |
| let totalLength = 0; | |
| if (cachedPage !== null) { | |
| totalLength = cachedPage.contents.split("\n").length; | |
| return "Total length: " + totalLength + " Contents: " + cachedPage.contents; | |
| } else { | |
| const exa = new Exa(process.env.EXASEARCH_API_KEY || ""); | |
| const result = await exa.getContents([url], { | |
| text: true, | |
| context: true, | |
| extras: { | |
| links: links, | |
| imageLinks: imageLinks, | |
| }, | |
| }); | |
| await updateCachedPageContents(url, result.context ?? ""); | |
| // Handle offset and numLines | |
| let finalContents = result.context ?? ""; | |
| let totalLength = finalContents.split("\n").length; | |
| if (offset > 0 || numLines < Infinity) { | |
| const lines = finalContents.split("\n"); | |
| const startLine = Math.min(offset, lines.length); | |
| const endLine = Math.min(startLine + numLines, lines.length); | |
| finalContents = lines.slice(startLine, endLine).join("\n"); | |
| } | |
| if (totalLength > finalContents.split("\n").length) { | |
| return ( | |
| "Total length: " + | |
| totalLength + | |
| ` (Tip): The following text content is truncated due to size (content includes lines ${offset} to lines ${offset + numLines}). Use an offset and the numLines parameter to view more. \n` + | |
| "Contents: " + | |
| finalContents | |
| ); | |
| } else { | |
| return "Total length: " + totalLength + " Contents: " + finalContents; | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/tools/getSiteContents.ts from lines 92 to 136, fix the return statement
on line 99 to return the string content instead of the object by accessing the
contents property of pageContents. Remove the console.log on line 114 or replace
it with proper logging as per project standards. Delete the commented-out dead
code on lines 133 to 135. Rename the local variable pageContents to avoid
shadowing the imported table name, choosing a distinct name like cachedPage or
similar.
2 participants
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Chores