diff --git a/src/tools/README.md b/docs/TOOLS.md similarity index 82% rename from src/tools/README.md rename to docs/TOOLS.md index 1439fdf9..952fa1e0 100644 --- a/src/tools/README.md +++ b/docs/TOOLS.md @@ -318,64 +318,26 @@ function updateMcpToolSetting(toolId: string, enabled: boolean) { 3. **Settings UI**: `ToolSettingsSection` component reads from the registry to generate UI 4. **Tool Execution**: `AutonomousAgentChainRunner.getAvailableTools()` filters tools based on settings -### Tool Execution Flow +## Tool Call Rendering Roots -1. Agent calls `getAvailableTools()` which: +React invariant #409 surfaced when tool-call banners attempted to render into React roots that had already been unmounted. To prevent this regression, the plugin routes all banner rendering through the shared manager in `src/components/chat-components/toolCallRootManager.tsx`. - - Gets enabled tool IDs from settings array (`autonomousAgentEnabledToolIds`) - - Calls `registry.getEnabledTools()` to get actual tool implementations - - Filters based on vault availability and user preferences +### Manager Responsibilities -2. Model adapter receives tool list and: +- Tracks `{ root, isUnmounting }` per message/tool call via `window.__copilotToolCallRoots`. +- `ensureToolCallRoot` finalises pending disposals and creates a new `createRoot` when needed. +- `renderToolCallBanner` renders `` into the managed root; components never call `root.render` directly. +- `removeToolCallRoot` and `cleanupMessageToolCallRoots` schedule unmounts on the next tick and drop entries only after disposal completes. +- `cleanupStaleToolCallRoots` purges message IDs older than one hour to avoid leaking historical roots. - - Generates tool descriptions for the system prompt - - Includes tool-specific instructions based on enabled tools +### Integration Notes -3. When tool is called: - - XML parsing extracts tool name and parameters - - Tool is executed via its `func` implementation - - Results are formatted and returned to the agent +`ChatSingleMessage` keeps `const rootsRef = useRef(getMessageToolCallRoots(messageId))`, which provides a stable registry for each message. The component delegates all lifecycle calls to the manager and snapshots `rootsRef.current` inside effect cleanup to satisfy `react-hooks/exhaustive-deps`. -## Benefits of This Architecture +### Verification -1. **Modularity**: Each tool is self-contained with metadata -2. **Extensibility**: New tools can be added without core changes -3. **Backward Compatibility**: Settings structure preserved while supporting new tools -4. **Dynamic UI**: Settings automatically adapt to registered tools -5. **Smart Prompts**: System prompts include only relevant tool instructions -6. **MCP Ready**: Architecture supports dynamic tool registration from external sources +Run the focused test to cover the streaming behaviour and tool-call integration: -## Testing Your Tools - -```typescript -// Test tool registration -const registry = ToolRegistry.getInstance(); -registry.clear(); -initializeBuiltinTools(vault); - -// Verify tool is registered -const allTools = registry.getAllTools(); -console.log( - "Registered tools:", - allTools.map((t) => t.metadata.id) -); - -// Test with settings -const enabledIds = new Set(["myNewTool", "localSearch"]); -const enabledTools = registry.getEnabledTools(enabledIds, true); -console.log( - "Enabled tools:", - enabledTools.map((t) => t.name) -); ``` - -This architecture provides a clean, extensible foundation for the tool system while maintaining simplicity and backward compatibility. - -## Summary - -The tool system's layered approach allows for: - -- Clear, comprehensive tool documentation at the schema level -- Model-agnostic instructions that work for most LLMs -- Targeted model-specific adaptations when necessary -- Easy extension for new tools without modifying core infrastructure +npm test -- src/components/chat-components/ChatSingleMessage.test.tsx +``` diff --git a/src/components/chat-components/ChatSingleMessage.tsx b/src/components/chat-components/ChatSingleMessage.tsx index de03515d..9bb6173b 100644 --- a/src/components/chat-components/ChatSingleMessage.tsx +++ b/src/components/chat-components/ChatSingleMessage.tsx @@ -1,5 +1,4 @@ import { ChatButtons } from "@/components/chat-components/ChatButtons"; -import { ToolCallBanner } from "@/components/chat-components/ToolCallBanner"; import { SourcesModal } from "@/components/modals/SourcesModal"; import { Tooltip, TooltipContent, TooltipTrigger } from "@/components/ui/tooltip"; import { @@ -9,6 +8,15 @@ import { ContextFolderBadge, } from "@/components/chat-components/ContextBadges"; import { InlineMessageEditor } from "@/components/chat-components/InlineMessageEditor"; +import { + cleanupMessageToolCallRoots, + cleanupStaleToolCallRoots, + ensureToolCallRoot, + getMessageToolCallRoots, + renderToolCallBanner, + removeToolCallRoot, + type ToolCallRootRecord, +} from "@/components/chat-components/toolCallRootManager"; import { USER_SENDER } from "@/constants"; import { cn } from "@/lib/utils"; import { parseToolCallMarkers } from "@/LLMProviders/chainRunner/utils/toolCallParser"; @@ -18,13 +26,6 @@ import { ChatMessage } from "@/types/message"; import { cleanMessageForCopy, insertIntoEditor } from "@/utils"; import { App, Component, MarkdownRenderer, MarkdownView, TFile } from "obsidian"; import React, { useCallback, useEffect, useRef, useState } from "react"; -import ReactDOM, { Root } from "react-dom/client"; - -declare global { - interface Window { - __copilotToolCallRoots?: Map>; - } -} const FOOTNOTE_SUFFIX_PATTERN = /^\d+-\d+$/; @@ -151,22 +152,9 @@ const ChatSingleMessage: React.FC = ({ ); // Store roots in a global map to preserve them across component instances - const getGlobalRootsMap = () => { - if (!window.__copilotToolCallRoots) { - window.__copilotToolCallRoots = new Map>(); - } - return window.__copilotToolCallRoots; - }; - - const getRootsForMessage = () => { - const globalMap = getGlobalRootsMap(); - if (!globalMap.has(messageId.current)) { - globalMap.set(messageId.current, new Map()); - } - return globalMap.get(messageId.current)!; - }; - - const rootsRef = useRef>(getRootsForMessage()); + const rootsRef = useRef>( + getMessageToolCallRoots(messageId.current) + ); const copyToClipboard = () => { if (!navigator.clipboard || !navigator.clipboard.writeText) { @@ -383,26 +371,9 @@ const ChatSingleMessage: React.FC = ({ currentIndex++; } else if (segment.type === "toolCall" && segment.toolCall) { const toolCallId = segment.toolCall.id; - const existingDiv = document.getElementById(`tool-call-${toolCallId}`); + let container = document.getElementById(`tool-call-${toolCallId}`); - if (existingDiv) { - // Update existing tool call - const root = rootsRef.current.get(toolCallId); - if (root && !isUnmountingRef.current) { - root.render( - - ); - } - currentIndex++; - } else { - // Create new tool call + if (!container) { const insertBefore = contentRef.current!.children[currentIndex]; const toolDiv = document.createElement("div"); toolDiv.className = "tool-call-container"; @@ -414,23 +385,22 @@ const ChatSingleMessage: React.FC = ({ contentRef.current!.appendChild(toolDiv); } - const root = ReactDOM.createRoot(toolDiv); - rootsRef.current.set(toolCallId, root); - - if (!isUnmountingRef.current) { - root.render( - - ); - } - currentIndex++; + container = toolDiv; } + + const rootRecord = ensureToolCallRoot( + messageId.current, + rootsRef.current, + toolCallId, + container as HTMLElement, + "render refresh" + ); + + if (!isUnmountingRef.current && !rootRecord.isUnmounting) { + renderToolCallBanner(rootRecord, segment.toolCall); + } + + currentIndex++; } }); @@ -445,18 +415,7 @@ const ChatSingleMessage: React.FC = ({ if (!currentToolCallIds.has(id)) { const element = document.getElementById(`tool-call-${id}`); if (element) { - const root = rootsRef.current.get(id); - if (root) { - // Defer unmounting to avoid React rendering conflicts - setTimeout(() => { - try { - root.unmount(); - } catch (error) { - console.debug("Error unmounting tool call root:", error); - } - rootsRef.current.delete(id); - }, 0); - } + removeToolCallRoot(messageId.current, rootsRef.current, id, "tool call removal"); element.remove(); } } @@ -474,29 +433,11 @@ const ChatSingleMessage: React.FC = ({ useEffect(() => { const currentComponentRef = componentRef; const currentMessageId = messageId.current; + const messageRootsSnapshot = rootsRef.current; // Clean up old message roots to prevent memory leaks (older than 1 hour) const cleanupOldRoots = () => { - const globalMap = getGlobalRootsMap(); - const oneHourAgo = Date.now() - 60 * 60 * 1000; - - globalMap.forEach((roots, msgId) => { - // Extract timestamp from message ID if it's in epoch format - const timestamp = parseInt(msgId); - if (!isNaN(timestamp) && timestamp < oneHourAgo) { - // Defer cleanup to avoid React rendering conflicts - setTimeout(() => { - roots.forEach((root) => { - try { - root.unmount(); - } catch { - // Ignore errors - } - }); - globalMap.delete(msgId); - }, 0); - } - }); + cleanupStaleToolCallRoots(); }; // Run cleanup on mount @@ -517,20 +458,7 @@ const ChatSingleMessage: React.FC = ({ // Only clean up roots if this is a temporary message (streaming message) // Permanent messages keep their roots to preserve tool call banners if (currentMessageId.startsWith("temp-")) { - const globalMap = getGlobalRootsMap(); - const messageRoots = globalMap.get(currentMessageId); - - if (messageRoots) { - messageRoots.forEach((root) => { - try { - root.unmount(); - } catch (error) { - // Ignore unmount errors during cleanup - console.debug("Error unmounting React root during cleanup:", error); - } - }); - globalMap.delete(currentMessageId); - } + cleanupMessageToolCallRoots(currentMessageId, messageRootsSnapshot, "component cleanup"); } }, 0); }; diff --git a/src/components/chat-components/toolCallRootManager.tsx b/src/components/chat-components/toolCallRootManager.tsx new file mode 100644 index 00000000..59349f7c --- /dev/null +++ b/src/components/chat-components/toolCallRootManager.tsx @@ -0,0 +1,231 @@ +import React from "react"; +import { createRoot, Root } from "react-dom/client"; + +import { ToolCallBanner } from "@/components/chat-components/ToolCallBanner"; +import type { ToolCallMarker } from "@/LLMProviders/chainRunner/utils/toolCallParser"; +import { logWarn } from "@/logger"; + +declare global { + interface Window { + __copilotToolCallRoots?: Map>; + } +} + +export interface ToolCallRootRecord { + root: Root; + isUnmounting: boolean; +} + +const STALE_ROOT_MAX_AGE_MS = 60 * 60 * 1000; + +/** + * Retrieve the global registry that keeps track of tool call React roots. + * The registry is stored on `window` to preserve state across component lifecycles. + */ +const getRegistry = (): Map> => { + if (!window.__copilotToolCallRoots) { + window.__copilotToolCallRoots = new Map>(); + } + + return window.__copilotToolCallRoots; +}; + +/** + * Remove the message entry from the registry when it no longer has active tool call roots. + */ +const pruneEmptyMessageEntry = ( + messageId: string, + messageRoots: Map +): void => { + if (messageRoots.size > 0) { + return; + } + + const registry = getRegistry(); + const currentRoots = registry.get(messageId); + + if (currentRoots === messageRoots) { + registry.delete(messageId); + } +}; + +/** + * Unmount a tool call root, mark it as inactive, and remove it from the registry. + */ +const disposeToolCallRoot = ( + messageId: string, + messageRoots: Map, + toolCallId: string, + record: ToolCallRootRecord, + logContext: string +): void => { + try { + record.root.unmount(); + } catch (error) { + logWarn(`Error unmounting tool call root during ${logContext}`, toolCallId, error); + } + + record.isUnmounting = false; + + if (messageRoots.get(toolCallId) === record) { + messageRoots.delete(toolCallId); + } + + pruneEmptyMessageEntry(messageId, messageRoots); +}; + +/** + * Schedule a deferred unmount for a tool call root while preventing duplicate requests. + */ +const scheduleToolCallRootDisposal = ( + messageId: string, + messageRoots: Map, + toolCallId: string, + record: ToolCallRootRecord, + logContext: string +): void => { + if (record.isUnmounting) { + return; + } + + record.isUnmounting = true; + + setTimeout(() => { + const registry = getRegistry(); + const currentRoots = registry.get(messageId); + const currentRecord = currentRoots?.get(toolCallId); + + if (!currentRoots || currentRecord !== record) { + record.isUnmounting = false; + pruneEmptyMessageEntry(messageId, messageRoots); + return; + } + + disposeToolCallRoot(messageId, currentRoots, toolCallId, currentRecord, logContext); + }, 0); +}; + +/** + * Ensure a React root exists for the provided tool call container and return the root record. + */ +export const ensureToolCallRoot = ( + messageId: string, + messageRoots: Map, + toolCallId: string, + container: HTMLElement, + logContext: string +): ToolCallRootRecord => { + let record = messageRoots.get(toolCallId); + + if (record?.isUnmounting) { + disposeToolCallRoot( + messageId, + messageRoots, + toolCallId, + record, + `${logContext} (finalizing stale root)` + ); + record = undefined; + } + + if (!record) { + record = { + root: createRoot(container), + isUnmounting: false, + }; + + messageRoots.set(toolCallId, record); + } + + return record; +}; + +/** + * Render the `ToolCallBanner` component into the provided root record. + */ +export const renderToolCallBanner = ( + record: ToolCallRootRecord, + toolCall: ToolCallMarker +): void => { + record.root.render( + + ); +}; + +/** + * Schedule the removal of a tool call root from a message root collection. + */ +export const removeToolCallRoot = ( + messageId: string, + messageRoots: Map, + toolCallId: string, + logContext: string +): void => { + const record = messageRoots.get(toolCallId); + + if (!record) { + return; + } + + scheduleToolCallRootDisposal(messageId, messageRoots, toolCallId, record, logContext); +}; + +/** + * Return (and create if necessary) the tool call root map for a specific message. + */ +export const getMessageToolCallRoots = (messageId: string): Map => { + const registry = getRegistry(); + let messageRoots = registry.get(messageId); + + if (!messageRoots) { + messageRoots = new Map(); + registry.set(messageId, messageRoots); + } + + return messageRoots; +}; + +/** + * Clean up tool call roots for messages whose identifiers encode timestamps older than the configured threshold. + */ +export const cleanupStaleToolCallRoots = (now: number = Date.now()): void => { + const registry = getRegistry(); + + registry.forEach((messageRoots, messageId) => { + const timestamp = Number.parseInt(messageId, 10); + + if (Number.isNaN(timestamp) || now - timestamp < STALE_ROOT_MAX_AGE_MS) { + return; + } + + messageRoots.forEach((record, toolCallId) => { + scheduleToolCallRootDisposal( + messageId, + messageRoots, + toolCallId, + record, + "stale message cleanup" + ); + }); + }); +}; + +/** + * Schedule cleanup for all tool call roots owned by a specific message. + */ +export const cleanupMessageToolCallRoots = ( + messageId: string, + messageRoots: Map, + logContext: string +): void => { + messageRoots.forEach((record, toolCallId) => { + scheduleToolCallRootDisposal(messageId, messageRoots, toolCallId, record, logContext); + }); +};