Agent Builder #74
Agent Builder #74
Conversation
|
Claude finished @tysonthomas9's task —— View job PR Review: Agent BuilderI've completed a comprehensive review of this large PR that introduces the "Agent Builder" functionality. Here's my detailed analysis: 📊 Overview
✅ Strengths1. Architecture & Design
2. Code Quality
3. Testing
4. Mini Apps Framework
|
WalkthroughThis PR introduces a comprehensive mini-apps framework for AI Chat, including Agent Storage Manager with IndexedDB persistence, Agent Studio Integration for custom agent management, and a full mini-app lifecycle system (registry, bridge, event bus, storage). It adds new tools (SaveResearchReport, SearchCustomAgents, CallCustomAgent, and six mini-app tools), updates BaseOrchestratorAgent to support custom agents, makes AgentRunner initialization async, and removes the prior structured response system. Extensive test coverage and UI components (Agent Studio View, Bridge, Schema Editor) complete the feature. Changes
Sequence Diagram(s)sequenceDiagram
participant DevTools as DevTools<br/>(AIChatPanel)
participant Bridge as AgentStudio<br/>Bridge
participant SPA as Agent Studio<br/>SPA
participant Storage as Storage<br/>(IndexedDB)
participant Framework as Agent<br/>Framework
DevTools->>Bridge: onAction(handler)
DevTools->>Bridge: install(webappId)
Bridge->>SPA: Runtime.addBinding
SPA->>SPA: window.__agentStudioBridge
User->>SPA: Save Agent
SPA->>Bridge: dispatch({type: 'save-agent'})
Bridge->>DevTools: handleBindingCalled
DevTools->>Storage: createAgent(config)
Storage-->>DevTools: success/error
DevTools->>Framework: AgentStudioIntegration.refreshAgents()
Framework->>Storage: getAllAgents()
Framework->>Framework: registerCustomAgent(stored)
Framework-->>DevTools: agents registered
DevTools->>Bridge: sendToSPA({action: 'agents-updated'})
Bridge->>SPA: Runtime.evaluate
SPA->>SPA: window.agentStudio.dispatch()
SPA-->>User: Show updated agents
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Key areas requiring careful review:
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Reject non-http(s) endpoints in validation
validateEndpoint currently accepts any URL that parses, including ftp or other unsupported schemes, because it only calls new URL(endpoint) and returns true on success. The newly added validation tests expect ftp URLs to be rejected, and allowing them means users can save endpoints DevTools cannot call, causing configuration errors and failing the added tests. Restricting the protocol to http/https (and rejecting ftp) matches the intended behaviour.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
🟠 Major comments (23)
front_end/panels/ai_chat/ui/model_selector/__tests__/ModelSelector.test.ts-308-310 (1)
308-310: Useassert.lengthOffor array length checks.Consistent with the codebase style for checking array-like lengths.
Based on static analysis hints, apply this diff:
const visibleOptions = selector.querySelectorAll('.model-option:not(.no-results)'); - assert.strictEqual(visibleOptions.length, 2, 'Should only show GPT models'); + assert.lengthOf(visibleOptions, 2, 'Should only show GPT models');front_end/panels/ai_chat/ui/model_selector/__tests__/ModelSelector.test.ts-359-361 (1)
359-361: Useassert.lengthOffor array length checks.Consistent with the codebase style for checking array-like lengths.
Based on static analysis hints, apply this diff:
const visibleOptions = selector.querySelectorAll('.model-option:not(.no-results)'); - assert.strictEqual(visibleOptions.length, 1); + assert.lengthOf(visibleOptions, 1);front_end/panels/ai_chat/ui/model_selector/__tests__/ModelSelector.test.ts-74-76 (1)
74-76: Useassert.lengthOffor array length checks.The codebase prefers
assert.lengthOffor checking array-like lengths.Based on static analysis hints, apply this diff:
const optionEls = select!.querySelectorAll('option'); - assert.strictEqual(optionEls.length, 3); + assert.lengthOf(optionEls, 3);front_end/panels/ai_chat/ui/model_selector/__tests__/ModelSelector.test.ts-21-27 (1)
21-27: Fix type definition formatting.Missing delimiters in the type definition according to the codebase style guide.
Based on static analysis hints, apply this diff:
function createSelector(options?: { - options?: ModelOption[]; - selected?: string; - disabled?: boolean; - preferAbove?: boolean; - forceSearchable?: boolean; + options?: ModelOption[], + selected?: string, + disabled?: boolean, + preferAbove?: boolean, + forceSearchable?: boolean, }): ModelSelector {front_end/panels/ai_chat/ui/model_selector/__tests__/ModelSelector.test.ts-12-19 (1)
12-19: Use the preferred DOM rendering helper.The ESLint rule flags direct DOM mutation. Consider using the
renderElementIntoDOMhelper if available in the codebase for better test isolation.Based on static analysis hints, apply this pattern if
renderElementIntoDOMis available:- beforeEach(() => { - container = document.createElement('div'); - document.body.appendChild(container); - }); - - afterEach(() => { - document.body.removeChild(container); - }); + beforeEach(() => { + container = renderElementIntoDOM('div'); + }); + + afterEach(() => { + container.remove(); + });Committable suggestion skipped: line range outside the PR's diff.
front_end/panels/ai_chat/ui/settings/__tests__/validation.test.ts-11-15 (1)
11-15: Useinterfaceand comma delimiters per project standards.The static analysis correctly identifies style issues. Additionally, verify whether
ModelOptionis already exported from the validation module to avoid duplication.Apply this diff to fix the style issues:
-type ModelOption = { - value: string; - label: string; - type?: string; -}; +interface ModelOption { + value: string, + label: string, + type?: string, +}If
ModelOptionis already exported fromvalidation.ts, import it instead:-import { - validateModelSelection, - validateApiKey, - validateEndpoint, -} from '../utils/validation.js'; - -type ModelOption = { - value: string; - label: string; - type?: string; -}; +import { + validateModelSelection, + validateApiKey, + validateEndpoint, + type ModelOption, +} from '../utils/validation.js';front_end/panels/ai_chat/ui/input/__tests__/InputBar.test.ts-23-64 (1)
23-64: Tighten typing forcreateInputBaroptions and avoidanycastsThe helper currently uses
(bar as any)for every property assignment, which triggersno-explicit-anyand weakens type safety. The member-delimiter-style rule also expects commas rather than semicolons in the options type literal.You can address both by (a) using an extended local type for the test and (b) switching delimiters to commas:
- function createInputBar(options?: { - placeholder?: string; - disabled?: boolean; - sendDisabled?: boolean; - modelOptions?: Array<{value: string; label: string}>; - selectedModel?: string; - modelSelectorDisabled?: boolean; - currentProvider?: string; - centered?: boolean; - imageInput?: { url: string; bytesBase64: string }; - }): InputBar { - const bar = document.createElement('ai-input-bar') as InputBar; + type TestInputBar = InputBar & { + placeholder?: string, + disabled?: boolean, + sendDisabled?: boolean, + modelOptions?: Array<{value: string; label: string}>, + selectedModel?: string, + modelSelectorDisabled?: boolean, + currentProvider?: string, + centered?: boolean, + imageInput?: { url: string; bytesBase64: string }, + }; + + function createInputBar(options?: { + placeholder?: string, + disabled?: boolean, + sendDisabled?: boolean, + modelOptions?: Array<{value: string; label: string}>, + selectedModel?: string, + modelSelectorDisabled?: boolean, + currentProvider?: string, + centered?: boolean, + imageInput?: { url: string; bytesBase64: string }, + }): InputBar { + const bar = document.createElement('ai-input-bar') as TestInputBar; if (options?.placeholder !== undefined) { - (bar as any).placeholder = options.placeholder; + bar.placeholder = options.placeholder; } if (options?.disabled !== undefined) { - (bar as any).disabled = options.disabled; + bar.disabled = options.disabled; } if (options?.sendDisabled !== undefined) { - (bar as any).sendDisabled = options.sendDisabled; + bar.sendDisabled = options.sendDisabled; } if (options?.modelOptions !== undefined) { - (bar as any).modelOptions = options.modelOptions; + bar.modelOptions = options.modelOptions; } if (options?.selectedModel !== undefined) { - (bar as any).selectedModel = options.selectedModel; + bar.selectedModel = options.selectedModel; } if (options?.modelSelectorDisabled !== undefined) { - (bar as any).modelSelectorDisabled = options.modelSelectorDisabled; + bar.modelSelectorDisabled = options.modelSelectorDisabled; } if (options?.currentProvider !== undefined) { - (bar as any).currentProvider = options.currentProvider; + bar.currentProvider = options.currentProvider; } if (options?.centered !== undefined) { - (bar as any).centered = options.centered; + bar.centered = options.centered; } if (options?.imageInput !== undefined) { - (bar as any).imageInput = options.imageInput; + bar.imageInput = options.imageInput; }This keeps the helper strongly typed while satisfying the lint rules.
front_end/panels/ai_chat/ui/input/__tests__/InputBar.test.ts-14-21 (1)
14-21: UserenderElementIntoDOMinstead of mutatingdocument.bodydirectlyThe test setup/teardown currently appends/removes
containerdirectly ondocument.body, which violates theno-document-body-mutationlint rule and bypasses the common helper used elsewhere.Recommend switching to
renderElementIntoDOMand simplifying teardown, e.g.:-import {raf} from '../../../../../testing/DOMHelpers.js'; +import {raf, renderElementIntoDOM} from '../../../../../testing/DOMHelpers.js'; beforeEach(() => { - container = document.createElement('div'); - document.body.appendChild(container); + container = document.createElement('div'); + renderElementIntoDOM(container); }); afterEach(() => { - document.body.removeChild(container); + container.remove(); });This keeps tests consistent with the shared DOM helpers and satisfies the lint rule.
Committable suggestion skipped: line range outside the PR's diff.
front_end/panels/ai_chat/ui/input/__tests__/ChatInput.test.ts-21-25 (1)
21-25: Fixmember-delimiter-styleviolations inoptionstypeESLint’s
@stylistic/member-delimiter-styleis complaining on lines 22–24. The current inline type uses a delimiter style that doesn’t match the project configuration. Align with the configured style (commas for multiline members), e.g.:- function createInput(options?: { - placeholder?: string; - disabled?: boolean; - value?: string; - }): ChatInput { + function createInput(options?: { + placeholder?: string, + disabled?: boolean, + value?: string, + }): ChatInput {This should clear the lint errors without changing runtime behavior.
front_end/panels/ai_chat/ui/input/__tests__/ChatInput.test.ts-303-322 (1)
303-322: Useassert.lengthOfand consider async behavior in rapid-input testESLint suggests using
assert.lengthOfinstead of checking.lengthmanually. You can appease the linter and keep the intent clear:- assert.strictEqual(values.length, 20, 'Should capture all input events'); + assert.lengthOf(values, 20, 'Should capture all input events');Also, note that this test assumes
inputchangeis fired synchronously for each underlyinginputevent. IfChatInputever starts debouncing or batching these events, the expectations here will need to be revisited. For now, the current assertions are fine given synchronous behavior.front_end/panels/ai_chat/ui/input/__tests__/ChatInput.test.ts-268-285 (1)
268-285: Fix unusedinitialHeightand consider strengthening auto-sizing assertionESLint flags
initialHeightas assigned but never used. You can either remove it or use it to assert actual auto-sizing behavior. A tighter (but still reasonable) assertion would be:- const textarea = getTextarea(input); - const initialHeight = textarea.offsetHeight; + const textarea = getTextarea(input); + const initialHeight = textarea.offsetHeight; @@ - // Height should have increased (or at least not be the same single-line height) - // Note: exact heights depend on styling - assert.isNotNaN(textarea.offsetHeight); + // Height should increase for multi-line content. + const newHeight = textarea.offsetHeight; + assert.isAbove(newHeight, initialHeight, 'auto-sizing should increase height for multi-line content');If you’re concerned about cross-environment flakiness, alternatively drop
initialHeightentirely and keep a weaker assertion, but that won’t really validate auto-sizing semantics.front_end/panels/ai_chat/ui/input/__tests__/ChatInput.test.ts-12-19 (1)
12-19: UserenderElementIntoDOMinstead of directdocument.bodymutationESLint is flagging
document.body.appendChild(container); tests generally should rely on the sharedrenderElementIntoDOMhelper instead of mutatingdocument.bodydirectly. You can import it alongsiderafand use it inbeforeEach:-import {raf} from '../../../../../testing/DOMHelpers.js'; +import {raf, renderElementIntoDOM} from '../../../../../testing/DOMHelpers.js'; @@ - beforeEach(() => { - container = document.createElement('div'); - document.body.appendChild(container); - }); + beforeEach(() => { + container = document.createElement('div'); + renderElementIntoDOM(container); + });If
renderElementIntoDOMalready handles cleanup, you may also be able to drop the explicitremoveChildinafterEach.front_end/panels/ai_chat/ui/SchemaEditor.ts-432-437 (1)
432-437: Security: Avoid using'*'as postMessage targetOrigin.Using
'*'allows any origin to receive the message, which could leak schema data to malicious frames. Specify an explicit origin or use a more restrictive pattern.// Send to parent window.parent.postMessage({ type: 'schema-change', schema: schema -}, '*'); +}, window.location.origin);Alternatively, if this is for cross-origin communication, document why
'*'is necessary and consider validating the recipient.front_end/panels/ai_chat/agent_framework/implementation/ConfiguredAgents.ts-18-20 (1)
18-20: Fix import order for consistency.The new imports should be grouped with similar imports to follow the established import order pattern in this file.
Apply this diff to fix import ordering:
+import { SaveResearchReportTool } from '../../tools/SaveResearchReportTool.js'; +import { SearchCustomAgentsTool } from '../../tools/SearchCustomAgentsTool.js'; +import { CallCustomAgentTool } from '../../tools/CallCustomAgentTool.js'; import { FetcherTool } from '../../tools/FetcherTool.js'; import { FinalizeWithCritiqueTool } from '../../tools/FinalizeWithCritiqueTool.js'; import { SchemaBasedExtractorTool } from '../../tools/SchemaBasedExtractorTool.js'; import { StreamlinedSchemaExtractorTool } from '../../tools/StreamlinedSchemaExtractorTool.js'; import { BookmarkStoreTool } from '../../tools/BookmarkStoreTool.js'; import { DocumentSearchTool } from '../../tools/DocumentSearchTool.js'; import { NavigateURLTool, PerformActionTool, GetAccessibilityTreeTool, SearchContentTool, NavigateBackTool, NodeIDsToURLsTool, TakeScreenshotTool, ScrollPageTool, WaitTool, RenderWebAppTool, GetWebAppDataTool, RemoveWebAppTool, CreateFileTool, UpdateFileTool, DeleteFileTool, ReadFileTool, ListFilesTool } from '../../tools/Tools.js'; import { UpdateTodoTool } from '../../tools/UpdateTodoTool.js'; import { ExecuteCodeTool } from '../../tools/ExecuteCodeTool.js'; import { HTMLToMarkdownTool } from '../../tools/HTMLToMarkdownTool.js'; import { ReadabilityExtractorTool } from '../../tools/ReadabilityExtractorTool.js'; +import { AgentStudioIntegration } from '../../core/AgentStudioIntegration.js'; import { ConfigurableAgentTool, ToolRegistry } from '../ConfigurableAgentTool.js'; import { ThinkingTool } from '../../tools/ThinkingTool.js'; -import { SaveResearchReportTool } from '../../tools/SaveResearchReportTool.js'; -import { SearchCustomAgentsTool } from '../../tools/SearchCustomAgentsTool.js'; -import { CallCustomAgentTool } from '../../tools/CallCustomAgentTool.js'; import { registerMCPMetaTools } from '../../mcp/MCPMetaTools.js'; +import { initializeMiniApps } from '../../mini_apps/MiniAppInitialization.js'; import { createDirectURLNavigatorAgentConfig } from './agents/DirectURLNavigatorAgent.js'; import { createResearchAgentConfig } from './agents/ResearchAgent.js'; ... -import { AgentStudioIntegration } from '../../core/AgentStudioIntegration.js'; -import { initializeMiniApps } from '../../mini_apps/MiniAppInitialization.js';Also applies to: 35-36
front_end/panels/ai_chat/ui/AIChatPanel.ts-93-94 (1)
93-94: Move import to the top with other UI imports.The
AgentStudioViewimport should be grouped with other UI imports at the top of the file (around lines 78-94) rather than placed in the middle after comments. This violates import ordering conventions.Apply this diff to fix the import order:
import { ChatView } from './ChatView.js'; import { type ChatMessage, ChatMessageEntity, type ImageInputData, type ModelChatMessage, State as ChatViewState } from '../models/ChatTypes.js'; import { HelpDialog } from './HelpDialog.js'; import { SettingsDialog } from './SettingsDialog.js'; import { EvaluationDialog } from './EvaluationDialog.js'; import { MODEL_PLACEHOLDERS } from '../core/Constants.js'; import * as Snackbars from '../../../ui/components/snackbars/snackbars.js'; +import { AgentStudioView } from './AgentStudioView.js'; // MCP integration import { MCPRegistry } from '../mcp/MCPRegistry.js'; import { getMCPConfig } from '../mcp/MCPConfig.js'; import { onMCPConfigChange } from '../mcp/MCPConfig.js'; import { MCPConnectorsCatalogDialog } from './mcp/MCPConnectorsCatalogDialog.js'; // Conversation history import { ConversationHistoryList } from './ConversationHistoryList.js'; -// Agent Studio -import { AgentStudioView } from './AgentStudioView.js';front_end/panels/ai_chat/tools/SaveResearchReportTool.ts-104-109 (1)
104-109: Avoid usinganytype in catch block.ESLint flags the use of
anyon line 104. Useunknownand proper type narrowing instead.- } catch (error: any) { - logger.error('Failed to save research report', { filename, error: error?.message }); + } catch (error: unknown) { + const errorMessage = error instanceof Error ? error.message : String(error); + logger.error('Failed to save research report', { filename, error: errorMessage }); return { success: false, - error: error?.message || 'Failed to save research report.' + error: errorMessage || 'Failed to save research report.' }; }front_end/panels/ai_chat/tools/SaveResearchReportTool.ts-5-8 (1)
5-8: Fix import ordering per ESLint rules.Static analysis indicates import ordering issues. Regular imports should be grouped separately from type-only imports, and regular imports should come first within their group.
-import { createLogger } from '../core/Logger.js'; -import type { Tool, LLMContext } from './Tools.js'; import { FileStorageManager } from './FileStorageManager.js'; import { FileContentViewer } from '../ui/FileContentViewer.js'; +import { createLogger } from '../core/Logger.js'; + +import type { Tool, LLMContext } from './Tools.js';front_end/panels/ai_chat/mini_apps/MiniAppRegistry.ts-169-172 (1)
169-172: Partial state cleanup missing on launch failure.If an error occurs after the webapp is rendered (e.g., during
controller.initialize()), the webapp and bridge remain installed but the instance is not tracked, leaving orphaned resources.Consider cleaning up partial state:
} catch (error) { logger.error(`Failed to launch mini app "${appId}":`, error); + // Attempt cleanup of any partial initialization + try { + if (bridge?.installed) { + await bridge.uninstall(); + } + if (webappId) { + const removeTool = new RemoveWebAppTool(); + await removeTool.execute({ webappId, reasoning: `Cleanup after failed launch: ${appId}` }); + } + } catch (cleanupError) { + logger.error(`Cleanup failed after launch error:`, cleanupError); + } throw error; }This requires declaring
bridgeandwebappIdwithletbefore the try block.Committable suggestion skipped: line range outside the PR's diff.
front_end/panels/ai_chat/core/BaseOrchestratorAgent.ts-43-46 (1)
43-46: Fire-and-forget async initialization silently fails without error handling.The
initializeConfiguredAgents()function lacks any try-catch blocks and performs potentially failure-prone operations likeregisterMCPMetaTools(),initializeMiniApps(), and multiple tool factory registrations. Calling it withvoidmeans errors are unhandled, which may result in incomplete tool initialization without detection.Add error logging:
// Initialize configured agents (including custom agents from Agent Studio) // Note: This is async but we don't await here to avoid blocking module load. // Custom agents will be loaded asynchronously and available after initial load. -void initializeConfiguredAgents(); +void initializeConfiguredAgents().catch(error => { + console.error('Failed to initialize configured agents:', error); +});front_end/panels/ai_chat/core/AgentTestRunner.ts-5-8 (1)
5-8: Tighten typing and address ESLint complaints (any, import order, member delimiters)A few small changes will satisfy the lints and improve typings without behavior change:
- Remove explicit
anyfromprocessResult(Line 183):- private processResult(result: ConfigurableAgentResult & { agentSession?: any }, state: TestExecutionState): AgentTestResult { + private processResult(result: ConfigurableAgentResult, state: TestExecutionState): AgentTestResult {
ConfigurableAgentTool.execute()can still return an object with extra fields; narrowing toConfigurableAgentResultis valid and removes theno-explicit-anyerror.
- Fix inline type literals in
convertChatMessage(Lines 223–231) to use commas instead of semicolons, matching projectmember-delimiter-style:- if (msg.entity === 'user') { - content = (msg as { text: string }).text || ''; - } else if (msg.entity === 'model') { - const modelMsg = msg as { answer?: string; toolName?: string }; + if (msg.entity === 'user') { + content = (msg as { text: string }).text || ''; + } else if (msg.entity === 'model') { + const modelMsg = msg as { answer?: string, toolName?: string }; content = modelMsg.answer || ''; toolName = modelMsg.toolName; } else if (msg.entity === 'tool_result') { - const toolMsg = msg as { resultText: string; toolName: string }; + const toolMsg = msg as { resultText: string, toolName: string }; content = toolMsg.resultText || ''; toolName = toolMsg.toolName; }
- Align imports with
import/order+ grouping rules (Lines 5–8). For example:-import { createLogger } from './Logger.js'; -import { ConfigurableAgentTool, type AgentToolConfig, type ConfigurableAgentResult, type CallCtx } from '../agent_framework/ConfigurableAgentTool.js'; -import type { ChatMessage } from '../models/ChatTypes.js'; +import type { ChatMessage } from '../models/ChatTypes.js'; + +import { createLogger } from './Logger.js'; +import { ConfigurableAgentTool, type AgentToolConfig, type ConfigurableAgentResult, type CallCtx } from '../agent_framework/ConfigurableAgentTool.js';This should satisfy both the “import should occur after type import” and “empty line between import groups” ESLint errors.
Also applies to: 183-212, 217-233
front_end/panels/ai_chat/testing/TestUtilities.ts-10-16 (1)
10-16: Test helpers look solid; clean up lint issues (import order, unused type, style)The helper implementations (mock tools, CallCtx/Agent configs, message builders, assertions) look consistent with the core types and appear correct for tests.
ESLint is still unhappy about a few things though:
- Import/ordering + inline type imports: re‑order the type imports at the top so they satisfy
import/orderand keep related types together (e.g. AgentSessionTypes before ChatTypes if that’s the configured order).- Unused
AgentMessagetype (Line 13): drop it from the import list if you don’t end up using it anywhere.member-delimiter-style&arrow-parens: several inline option types (e.g. functionoptionsparams) use;instead of,, and arrow functions likemessages.filter((m) => ...)still have parens around a single argument. Running the repo’s ESLint with--fixon this file should clean these up automatically.No behavior changes are needed; this is purely to get the lints passing.
Also applies to: 24-31, 59-69, 147-164, 183-225, 288-354, 359-391, 400-417
front_end/panels/ai_chat/ui/__tests__/test-utils.ts-470-481 (1)
470-481: FixsetupChatViewstate updates and removeanycast
setupChatViewcurrently:
- Stores a fixed
fullDatasnapshot (Line 511).updateDataalways mergesnewDatainto that initial snapshot (Line 519), so later updates can unintentionally reset previously updated fields.- Uses
(view as any).data, which triggersno-explicit-any.You can address all three at once by tracking
currentDataand strongly typing the element:export interface ChatViewData { messages: ChatMessage[]; - state: 'idle' | 'loading' | 'error'; + state: 'idle' | 'loading' | 'error'; isTextInputEmpty: boolean; onSendMessage: (text: string, imageInput?: ImageInputData | null) => void; onPromptSelected: (prompt: string, agentType?: string) => void; selectedAgent?: string; selectedModel?: string; modelOptions?: Array<{ value: string; label: string }>; onModelChanged?: (model: string) => void; isProcessing?: boolean; } + +type ChatViewElement = HTMLElement & { data: ChatViewData }; @@ export async function setupChatView(data?: Partial<ChatViewData>): Promise<{ - view: HTMLElement; + view: ChatViewElement; cleanup: () => void; updateData: (newData: Partial<ChatViewData>) => Promise<void>; }> { - const { element: view, cleanup } = renderToBody<HTMLElement>('devtools-chat-view'); - - const fullData = createChatViewData(data); - (view as any).data = fullData; + const { element: view, cleanup } = renderToBody<ChatViewElement>('devtools-chat-view'); + + let currentData = createChatViewData(data); + view.data = currentData; await raf(); @@ view, cleanup, updateData: async (newData: Partial<ChatViewData>) => { - (view as any).data = createChatViewData({ ...fullData, ...newData }); + currentData = createChatViewData({ ...currentData, ...newData }); + view.data = currentData; await raf(); }, }; }This gives
updateDataproper incremental semantics and removes the explicitany.Also applies to: 486-499, 504-523
front_end/panels/ai_chat/ui/__tests__/test-utils.ts-10-25 (1)
10-25: Align imports with project ESLint rules (es-modules-import, inline type imports, order)The top‑level imports are tripping several custom ESLint rules:
rulesdir/es-modules-import:../../../../testing/DOMHelpers.jsshould be imported using the project’s “namespace” pattern (e.g.import * as DOMHelpers from '...';) rather than pulling named exports directly. Mirror whatever pattern other tests in this repo use forDOMHelpersand then useDOMHelpers.raf/DOMHelpers.doubleRafinstead of named imports.rulesdir/inline-type-imports: types and values fromChatTypes/AgentSessionTypesshould be combined in a single import using thetypekeyword. For example:-import {ChatMessageEntity, State} from '../../models/ChatTypes.js'; -import type { - ChatMessage, - UserChatMessage, - ModelChatMessage, - ToolResultMessage as ChatToolResultMessage, - AgentSessionMessage, - ImageInputData, -} from '../../models/ChatTypes.js'; +import { + ChatMessageEntity, + State, + type ChatMessage, + type UserChatMessage, + type ModelChatMessage, + type ToolResultMessage as ChatToolResultMessage, + type AgentSessionMessage, + type ImageInputData, +} from '../../models/ChatTypes.js';And similarly for
AgentSession,AgentMessage,ToolCallMessage,ToolResultMessagefromAgentSessionTypes.
import/order: once combined, you may need to tweak the relative ordering of these imports to match the configured groups.These are stylistic but enforced as errors, so worth fixing before landing.
Committable suggestion skipped: line range outside the PR's diff.
🟡 Minor comments (11)
front_end/panels/ai_chat/ui/ChatView.ts-596-596 (1)
596-596: Remove theanycast and narrow the type forrenderModelMessage.The explicit
as anycast bypasses TypeScript type checking. WhilerenderModelMessageonly requires the baseModelChatMessagetype (which includesaction,answer,reasoning, anderror), themodelMessagevariable is typed as an intersection with additional properties needed for the tool call rendering path. Extract the base type before passing torenderModelMessage:if (isFinal) { - return renderModelMessage(modelMessage as any, this.#markdownRenderer); + const baseMessage: ModelChatMessage = modelMessage; + return renderModelMessage(baseMessage, this.#markdownRenderer);This preserves type safety and clarifies that only the base type is needed for final answer rendering.
Committable suggestion skipped: line range outside the PR's diff.
front_end/panels/ai_chat/agent_framework/__tests__/ConfigurableAgentTool.test.ts-229-244 (1)
229-244: Test appears incomplete - descriptor is unused.The test retrieves a descriptor but doesn't assert anything about it. Either add assertions or remove the unused assignment:
it('registers agent descriptor on creation', async () => { const config = createMockAgentToolConfig({ name: 'descriptor_agent', version: '1.0.0', }); new ConfigurableAgentTool(config); // Wait for async registration await new Promise(resolve => setTimeout(resolve, 10)); const descriptor = await AgentDescriptorRegistry.getDescriptor('descriptor_agent'); - // Descriptor registration is async, may or may not be available immediately - // This test verifies the pattern is followed + // Verify descriptor was registered (may be null if async registration not complete) + // At minimum, this verifies no errors are thrown during registration + assert.isOk(true); // Placeholder - consider adding meaningful assertion });Committable suggestion skipped: line range outside the PR's diff.
front_end/panels/ai_chat/ui/SchemaEditor.ts-10-16 (1)
10-16:SchemaPropertyinterface should use a distinct name to avoid future conflicts.Three different
SchemaPropertyinterfaces exist in the ai_chat module with incompatible structures:
SchemaEditor.ts:10: includesnameandrequiredfieldsAgentStorageManager.ts:17: lacksnameandrequiredfieldsSchemaBasedExtractorTool.ts:1341: different structure withformatand recursivepropertiesWhile
SchemaEditor.tsis currently isolated, this naming convention will create maintenance burden and potential conflicts if the file is integrated with other components. Rename this toEditorSchemaPropertyor another distinct name.front_end/panels/ai_chat/agent_framework/__tests__/AgentRunner.core.test.ts-667-690 (1)
667-690: LLM failure test doesn't restore original getInstance.The test directly overwrites
LLMClient.getInstanceand setsmockLLMCleanupto an empty function, which doesn't restore the original implementation. This could leak state to subsequent tests.Store and restore the original:
it('terminates with error on LLM failure', async () => { // Make LLM throw an error + const originalGetInstance = (LLMClient as any).getInstance; (LLMClient as any).getInstance = () => ({ call: async () => { throw new Error('LLM service unavailable'); }, parseResponse: () => ({ type: 'error', error: 'n/a' }), }); - mockLLMCleanup = () => {}; + mockLLMCleanup = () => { + (LLMClient as any).getInstance = originalGetInstance; + };front_end/panels/ai_chat/mini_apps/apps/agent_studio/AgentStudioMiniApp.ts-257-282 (1)
257-282: Unsafe type casting on action arguments.The
executeActionmethod uses aggressive type casting without validation, which can lead to runtime errors if the caller provides malformed arguments.Consider validating arguments before casting or using a more defensive approach:
async executeAction(actionName: string, args: unknown): Promise<unknown> { - const argsObj = args as Record<string, unknown>; + const argsObj = (args && typeof args === 'object') ? args as Record<string, unknown> : {}; switch (actionName) { case 'select-agent': + if (typeof argsObj.name !== 'string') { + throw new Error('select-agent requires a string "name" argument'); + } return this.handleSelectAgentAction(argsObj.name as string);front_end/panels/ai_chat/agent_framework/__tests__/AgentRunner.core.test.ts-443-484 (1)
443-484: Potential race condition in abort test.The test relies on a 20ms delay before aborting, which may be flaky on slower CI environments or under load. The
slowToolhas a 100ms delay, but timing-based tests can be unreliable.Consider using a more deterministic approach:
- // Create a slow tool that gives us time to abort - const slowTool = createMockTool('slow_tool', async () => { - await delay(100); - return { completed: true }; - }); + // Create a tool that waits for abort signal + let resolveToolExecution: () => void; + const toolPromise = new Promise<void>(resolve => { resolveToolExecution = resolve; }); + const slowTool = createMockTool('slow_tool', async () => { + await toolPromise; + return { completed: true }; + }); // ... later, abort immediately after starting + controller.abort(); + resolveToolExecution!(); // Allow tool to complete after abortCommittable suggestion skipped: line range outside the PR's diff.
front_end/panels/ai_chat/tools/mini_app/ListMiniAppsTool.ts-64-67 (1)
64-67: ConfusingincludeRunningfilter logic.When
includeRunning === false, the code filters to only return apps that are NOT running. The naming and logic are counterintuitive—includeRunning: falsemeans "exclude running apps" rather than "only return running apps."Consider renaming for clarity:
export interface ListMiniAppsArgs { - includeRunning?: boolean; + excludeRunning?: boolean; // If true, exclude currently running apps }- // Filter to only running apps if requested - const result = args.includeRunning === false - ? apps.filter(app => !app.isRunning) - : apps; + // Filter out running apps if requested + const result = args.excludeRunning + ? apps.filter(app => !app.isRunning) + : apps;Also update the schema description to match:
- description: 'If false, only return apps that are not currently running. Default: true (include all apps).', + description: 'If true, exclude apps that are currently running. Default: false (include all apps).',Committable suggestion skipped: line range outside the PR's diff.
front_end/panels/ai_chat/mini_apps/apps/agent_studio/AgentStudioMiniApp.ts-288-343 (1)
288-343: Type assertions in SPA action handling may cause runtime issues.Lines 298, 309, and 324 use inline type assertions (
action as SPAToDevToolsAction & {...}) without validating that the required properties exist. If the SPA sends malformed data, this will fail silently or throw cryptic errors.Add property validation before accessing:
case 'select-agent': { - const actionData = action as SPAToDevToolsAction & { name: string; id: string | null; isBuiltIn: boolean }; - await this.handleSelectAgent(actionData.name, actionData.id, actionData.isBuiltIn); + const { name, id, isBuiltIn } = action as Record<string, unknown>; + if (typeof name !== 'string') { + logger.warn('select-agent action missing name property'); + break; + } + await this.handleSelectAgent(name, id as string | null, Boolean(isBuiltIn)); break; }front_end/panels/ai_chat/mini_apps/MiniAppRegistry.ts-343-354 (1)
343-354: Potential duplicate 'ready' signal race condition.Both the
DOMContentLoadedlistener and thedocument.readyStatecheck can fire the 'ready' signal. IfDOMContentLoadedfires during the 100ms delay of the secondsetTimeout, both signals will be sent.Consider using a flag to ensure the ready signal is only sent once:
+ let __readySent = false; + function sendReady() { + if (__readySent) return; + __readySent = true; + window.${bindingName}(JSON.stringify({ type: 'ready' })); + } + // Signal that mini app is ready window.addEventListener('DOMContentLoaded', function() { - setTimeout(function() { - window.${bindingName}(JSON.stringify({ type: 'ready' })); - }, 100); + setTimeout(sendReady, 100); }); // If DOM is already loaded, signal ready immediately if (document.readyState !== 'loading') { - setTimeout(function() { - window.${bindingName}(JSON.stringify({ type: 'ready' })); - }, 100); + setTimeout(sendReady, 100); }front_end/panels/ai_chat/core/AgentStudioIntegration.ts-277-285 (1)
277-285: Stale agents remain callable after "unregistration".The
unregisterCustomAgentmethod only removes the agent from the localregisteredCustomAgentstracking Set (line 282) but cannot remove it fromToolRegistrysince the registry has no public unregister capability. Agents registered viaToolRegistry.registerToolFactory()at line 268 persist in ToolRegistry's internal maps and remain callable despite being marked as unregistered locally.Consider adding a TODO to track implementing proper unregistration support:
private static unregisterCustomAgent(name: string): void { registeredCustomAgents.delete(name); logger.info(`Marked custom agent as unregistered: ${name}`); - // Note: Actual removal from ToolRegistry would require adding an unregister method + // TODO: Implement ToolRegistry.unregisterTool() to properly remove agents + // Currently deleted agents remain in ToolRegistry until page reload }front_end/panels/ai_chat/core/AgentTestRunner.ts-131-143 (1)
131-143: Differentiate timeout from manual cancellation in error handlingRight now any aborted run is treated as a timeout (Line 135), even when
cancelTest()is called. That produces misleadingerrortext andterminationReasonfor user‑initiated cancels.Consider tracking a manual‑cancel flag in the execution state and using it in the catch branch:
interface TestExecutionState { abortController: AbortController; startTime: number; messages: TestMessage[]; - iterations: number; + iterations: number; + cancelledManually: boolean; } @@ const state: TestExecutionState = { abortController: new AbortController(), startTime: Date.now(), messages: [], - iterations: 0, + iterations: 0, + cancelledManually: false, }; @@ cancelTest(): boolean { if (this.currentTest) { - this.currentTest.abortController.abort(); + this.currentTest.cancelledManually = true; + this.currentTest.abortController.abort(); logger.info('Test execution cancelled'); return true; } @@ - if (state.abortController.signal.aborted) { - return { - success: false, - error: 'Test execution timed out', - messages: state.messages, - iterations: state.iterations, - duration, - terminationReason: 'timeout', - }; - } + if (state.abortController.signal.aborted) { + const terminationReason = state.cancelledManually ? 'cancelled' : 'timeout'; + return { + success: false, + error: state.cancelledManually ? 'Test execution cancelled' : 'Test execution timed out', + messages: state.messages, + iterations: state.iterations, + duration, + terminationReason, + }; + }Also applies to: 164-171, 46-51, 73-80
| it('returns false for invalid URLs', () => { | ||
| assert.isFalse(validateEndpoint('not-a-url')); | ||
| assert.isFalse(validateEndpoint('just text')); | ||
| assert.isFalse(validateEndpoint('ftp://invalid-scheme.com')); // ftp is valid URL though... | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Locate the validateEndpoint implementation
find . -type f -name "*.ts" -o -name "*.js" | xargs grep -l "validateEndpoint" | head -20Repository: BrowserOperator/browser-operator-core
Length of output: 218
🏁 Script executed:
# Search for the validateEndpoint function definition
rg "function validateEndpoint|export.*validateEndpoint|const validateEndpoint" -A 10Repository: BrowserOperator/browser-operator-core
Length of output: 901
🏁 Script executed:
cat -n front_end/panels/ai_chat/ui/settings/__tests__/validation.test.ts | sed -n '124,128p'Repository: BrowserOperator/browser-operator-core
Length of output: 376
🏁 Script executed:
# Check if there are any other references to scheme validation or restrictions in the codebase
rg "http|https|scheme" front_end/panels/ai_chat/ui/settings/ -iRepository: BrowserOperator/browser-operator-core
Length of output: 4589
Fix the test assertion for FTP URLs or restrict scheme validation to HTTP/HTTPS.
The test at line 127 expects validateEndpoint('ftp://invalid-scheme.com') to return false, but the implementation accepts any valid URL scheme via the new URL() constructor. Since FTP is a valid URL scheme, this test assertion will fail. The inline comment even acknowledges the contradiction: "ftp is valid URL though..."
Either restrict the implementation to accept only HTTP/HTTPS schemes, or change the assertion to assert.isTrue if all valid URL schemes should be accepted.
🤖 Prompt for AI Agents
In front_end/panels/ai_chat/ui/settings/__tests__/validation.test.ts around
lines 124-128, the test expects validateEndpoint('ftp://invalid-scheme.com') to
be false but FTP is a valid URL so the assertion is incorrect; either change the
test to assert.isTrue for FTP if the validator should accept all URL schemes, or
update the validateEndpoint implementation to explicitly restrict allowed
schemes to http/https (and then keep the test as assert.isFalse); pick one
approach and make the corresponding change to the test and/or implementation so
tests and validation rules are consistent.
Summary by CodeRabbit
Release Notes
New Features
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.