Add support for web asset rendering and virtual file system #55
Conversation
|
Claude finished @tysonthomas9's task —— View job PR Review - Web Asset Rendering and Virtual File SystemReviewing the pull request for code quality, bugs, performance, security, and test coverage. Todo List:
Overall AssessmentThis PR adds a comprehensive virtual file system and web application rendering capabilities to the AI chat interface. The implementation includes 8 new tools and updates to several agents. Here's my detailed review: ✅ Strengths1. Clean Architecture & Design
2. Robust Error Handling
3. Security Considerations
|
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for web application rendering and virtual file system capabilities to the AI chat tools. The changes introduce two main feature sets: web app rendering tools for creating interactive UI components in iframes, and file management tools for session-scoped virtual file storage.
- Web app rendering system with tools to create, manage, and extract data from full-screen iframe applications
- Virtual file system using IndexedDB for session-scoped file storage and management
- Integration of both tool sets across all existing agent configurations
Reviewed Changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| front_end/panels/ai_chat/tools/RenderWebAppTool.ts | Core tool for rendering interactive web applications in full-screen iframes |
| front_end/panels/ai_chat/tools/GetWebAppDataTool.ts | Tool for extracting form data from rendered web applications |
| front_end/panels/ai_chat/tools/RemoveWebAppTool.ts | Tool for cleaning up rendered web applications |
| front_end/panels/ai_chat/tools/FileStorageManager.ts | IndexedDB-based storage manager for virtual file system |
| front_end/panels/ai_chat/tools/CreateFileTool.ts | Tool for creating files in virtual file system |
| front_end/panels/ai_chat/tools/UpdateFileTool.ts | Tool for updating existing files in virtual file system |
| front_end/panels/ai_chat/tools/ReadFileTool.ts | Tool for reading files from virtual file system |
| front_end/panels/ai_chat/tools/ListFilesTool.ts | Tool for listing files in virtual file system |
| front_end/panels/ai_chat/tools/DeleteFileTool.ts | Tool for deleting files from virtual file system |
| front_end/panels/ai_chat/tools/Tools.ts | Exports for the new web app and file management tools |
| front_end/panels/ai_chat/core/BaseOrchestratorAgent.ts | Integration of new tools into orchestrator agents |
| front_end/panels/ai_chat/agent_framework/implementation/agents/*.ts | Addition of new tools to various agent configurations |
| front_end/panels/ai_chat/agent_framework/implementation/ConfiguredAgents.ts | Registration of new tool factories |
| front_end/panels/ai_chat/BUILD.gn | Build system updates for new source files |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds an IndexedDB-backed session file store and CRUDL file tools, webapp render/data/remove tools, UI for viewing/re-rendering webapps, unit tests for new components, and registers/exports the new tools across agent configs and build manifests. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Orchestrator
participant Agent
participant FileTool as File Tool (CRUDL)
participant FSM as FileStorageManager
participant IDB as IndexedDB
User->>Orchestrator: Start task
Orchestrator->>Agent: System prompt + availableTools (incl. file/webapp)
Agent->>FileTool: create_file / read_file / update_file / list_files / delete_file
FileTool->>FSM: operation(args)
FSM->>IDB: transaction (put/get/delete/index)
IDB-->>FSM: result / error
FSM-->>FileTool: StoredFile / FileSummary[] / error
FileTool-->>Agent: tool result
Agent-->>Orchestrator: progress / artifacts
Orchestrator-->>User: outputs / next steps
sequenceDiagram
autonumber
actor User
participant Agent
participant Render as RenderWebAppTool
participant Page as Primary Page Target
participant Iframe as WebApp Iframe
participant GetData as GetWebAppDataTool
participant Remove as RemoveWebAppTool
User->>Agent: Request interactive webapp
Agent->>Render: render_webapp(html, css, js, reasoning)
Render->>Page: runtime evaluate -> inject fullscreen iframe
Page-->>Render: {webappId}
Render-->>Agent: {success, webappId}
Agent->>GetData: get_webapp_data(webappId, waitForSubmit?)
GetData->>Page: evaluate in page -> read iframe form fields
Page-->>GetData: {formData}
GetData-->>Agent: {success, formData}
Agent->>Remove: remove_webapp(webappId)
Remove->>Page: evaluate -> remove iframe
Page-->>Remove: {removed:[webappId]}
Remove-->>Agent: {success}
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
front_end/panels/ai_chat/tools/GetWebAppDataTool.ts (1)
124-131: Fix literal interpolation in error message.The returned error string still contains the literal
${webappId}instead of the actual ID, because it’s wrapped in single quotes inside the evaluated script. Concatenate the value explicitly so callers see the real iframe ID.if (!iframe) { return { success: false, - error: 'Webapp iframe not found with ID: ${webappId}' + error: 'Webapp iframe not found with ID: ' + ${JSON.stringify(webappId)} }; }
🧹 Nitpick comments (13)
front_end/panels/ai_chat/tools/DeleteFileTool.ts (1)
26-39: Harden input schema, use safer error typing, and optionally record reasoning
- Add schema constraints (no path separators, minLength, no extra props).
- Prefer
unknownin catch; normalize error message.- Optionally log the reasoning for auditability.
schema = { type: 'object', properties: { fileName: { type: 'string', - description: 'Name of the file to delete' + description: 'Name of the file to delete', + minLength: 1, + pattern: '^[^/\\\\]+$' }, reasoning: { type: 'string', - description: 'Explanation for why the file can be safely deleted' + description: 'Explanation for why the file can be safely deleted', + minLength: 1 } }, - required: ['fileName', 'reasoning'] + required: ['fileName', 'reasoning'], + additionalProperties: false }; async execute(args: DeleteFileArgs, _ctx?: LLMContext): Promise<DeleteFileResult> { - logger.info('Executing delete file', { fileName: args.fileName }); + logger.info('Executing delete file', { fileName: args.fileName, reasoning: args.reasoning }); const manager = FileStorageManager.getInstance(); try { await manager.deleteFile(args.fileName); return { success: true, message: `Deleted file "${args.fileName}".` }; - } catch (error: any) { - logger.error('Failed to delete file', { fileName: args.fileName, error: error?.message }); + } catch (error: unknown) { + const message = error instanceof Error ? error.message : String(error); + logger.error('Failed to delete file', { fileName: args.fileName, error: message }); return { success: false, - error: error?.message || 'Failed to delete file.' + error: message || 'Failed to delete file.' }; } }Also applies to: 41-58
front_end/panels/ai_chat/tools/FileStorageManager.ts (2)
9-16: Add size limits and handle duplicate-create race more gracefully
- Guard against very large file writes to avoid memory/storage issues.
- Map IDB
ConstraintErrorto a friendly "already exists" message in create (pre-check is still TOCTOU).const DATABASE_NAME = 'ai_chat_agent_files'; const DATABASE_VERSION = 1; const OBJECT_STORE_NAME = 'files'; const INDEX_SESSION_ID = 'sessionId'; const INDEX_FILE_NAME = 'fileName'; const INDEX_CREATED_AT = 'createdAt'; const INDEX_SESSION_FILE_NAME = 'sessionId_fileName'; +const MAX_FILE_SIZE_BYTES = 5 * 1024 * 1024; // 5MB cap; make configurable if needed @@ async createFile(fileName: string, content: string, mimeType = 'text/plain'): Promise<StoredFile> { const validation = this.validateFileName(fileName); if (!validation.valid) { throw new Error(validation.error || 'Invalid file name'); } const db = await this.ensureDatabase(); if (await this.fileExists(fileName)) { throw new Error(`File "${fileName}" already exists in the current session.`); } const now = Date.now(); + const size = this.calculateSize(content); + if (size > MAX_FILE_SIZE_BYTES) { + throw new Error(`File content exceeds maximum allowed size (${MAX_FILE_SIZE_BYTES} bytes).`); + } const file: StoredFile = { id: this.generateFileId(), sessionId: this.sessionId, fileName, content, mimeType, createdAt: now, updatedAt: now, - size: this.calculateSize(content), + size, }; const transaction = db.transaction(OBJECT_STORE_NAME, 'readwrite'); const store = transaction.objectStore(OBJECT_STORE_NAME); - await this.requestToPromise(store.add(file)); + try { + await this.requestToPromise(store.add(file)); + } catch (err) { + // Handle concurrent creates that race the pre-existence check + if (err instanceof DOMException && err.name === 'ConstraintError') { + throw new Error(`File "${fileName}" already exists in the current session.`); + } + throw err; + } await this.transactionComplete(transaction); logger.info('Created file', { fileName, fileId: file.id, size: file.size }); return file; } @@ async updateFile(fileName: string, content: string, append = false): Promise<StoredFile> { + const validation = this.validateFileName(fileName); + if (!validation.valid) { + throw new Error(validation.error || 'Invalid file name'); + } const db = await this.ensureDatabase(); const existing = await this.getFileRecord(fileName); if (!existing) { throw new Error(`File "${fileName}" was not found in the current session.`); } const newContent = append ? `${existing.content}${content}` : content; + const newSize = this.calculateSize(newContent); + if (newSize > MAX_FILE_SIZE_BYTES) { + throw new Error(`File content exceeds maximum allowed size (${MAX_FILE_SIZE_BYTES} bytes).`); + } const updated: StoredFile = { ...existing, content: newContent, updatedAt: Date.now(), - size: this.calculateSize(newContent), + size: newSize, }; const transaction = db.transaction(OBJECT_STORE_NAME, 'readwrite'); const store = transaction.objectStore(OBJECT_STORE_NAME); await this.requestToPromise(store.put(updated)); await this.transactionComplete(transaction);Also applies to: 63-95, 97-119
121-139: Validate fileName consistently in delete/read pathsApply the same filename validation for consistent error messages and safety.
async deleteFile(fileName: string): Promise<void> { + const validation = this.validateFileName(fileName); + if (!validation.valid) { + throw new Error(validation.error || 'Invalid file name'); + } const db = await this.ensureDatabase(); const existing = await this.getFileRecord(fileName); if (!existing) { throw new Error(`File "${fileName}" was not found in the current session.`); } @@ async readFile(fileName: string): Promise<StoredFile | null> { + const validation = this.validateFileName(fileName); + if (!validation.valid) { + throw new Error(validation.error || 'Invalid file name'); + } const record = await this.getFileRecord(fileName); return record || null; }Also applies to: 166-177
front_end/panels/ai_chat/tools/CreateFileTool.ts (1)
18-24: Strengthen schema, safer error typing, and return richer metadata
- Add schema constraints and disallow extra props.
- Use
unknownin catch and normalize message.- Return
sizeandmimeTypeto avoid callers parsing the message.- Optionally log reasoning for audit.
export interface CreateFileResult { success: boolean; fileId?: string; fileName?: string; + size?: number; + mimeType?: string; message?: string; error?: string; } @@ schema = { type: 'object', properties: { fileName: { type: 'string', - description: 'Unique name of the file to create (no path separators)' + description: 'Unique name of the file to create (no path separators)', + minLength: 1, + pattern: '^[^/\\\\]+$' }, content: { type: 'string', description: 'Content to write to the file' }, mimeType: { type: 'string', description: 'Optional MIME type describing the content (default: text/plain)' }, reasoning: { type: 'string', - description: 'Explanation for why this file is being created for the user' + description: 'Explanation for why this file is being created for the user', + minLength: 1 } }, - required: ['fileName', 'content', 'reasoning'] + required: ['fileName', 'content', 'reasoning'], + additionalProperties: false }; async execute(args: CreateFileArgs, _ctx?: LLMContext): Promise<CreateFileResult> { - logger.info('Executing create file', { fileName: args.fileName }); + logger.info('Executing create file', { fileName: args.fileName, reasoning: args.reasoning }); const manager = FileStorageManager.getInstance(); try { const file: StoredFile = await manager.createFile(args.fileName, args.content, args.mimeType); return { success: true, fileId: file.id, fileName: file.fileName, + size: file.size, + mimeType: file.mimeType, message: `Created file "${file.fileName}" (${file.size} bytes).` }; - } catch (error: any) { - logger.error('Failed to create file', { fileName: args.fileName, error: error?.message }); + } catch (error: unknown) { + const message = error instanceof Error ? error.message : String(error); + logger.error('Failed to create file', { fileName: args.fileName, error: message }); return { success: false, - error: error?.message || 'Failed to create file.' + error: message || 'Failed to create file.' }; } }Also applies to: 30-51, 53-73
front_end/panels/ai_chat/core/BaseOrchestratorAgent.ts (2)
318-326: Document webapp tools in SEARCH agent prompt.The SEARCH agent configuration adds
RenderWebAppTool,GetWebAppDataTool, andRemoveWebAppToolto the available tools, but the SEARCH system prompt (lines 55-64) does not mention these tools or provide guidance on when to use them. This could lead to the agent not utilizing these capabilities effectively.Consider adding a section to the SEARCH system prompt explaining the webapp rendering capabilities:
- Use the file management tools ('create_file', 'update_file', 'read_file', 'list_files') to coordinate multi-step fact-finding. Persist subtask outputs as you go, read existing files before launching overlapping searches, and append incremental findings rather than duplicating effort. +- Use webapp rendering tools ('render_webapp', 'get_webapp_data', 'remove_webapp') when you need to collect structured input from users through forms or interactive interfaces (e.g., collecting email addresses, gathering survey responses, requesting additional search parameters). - If the user pivots into broad synthesis or long-form reporting, switch to the 'research_agent'.
342-349: Document webapp tools in DEEP_RESEARCH agent prompt.Similar to the SEARCH agent, the DEEP_RESEARCH configuration adds webapp tools but the system prompt (lines 66-202) does not document their purpose or usage scenarios. The detailed file coordination instructions (lines 130-135) provide good guidance for file tools, but webapp tools are entirely undocumented.
Add guidance for webapp tools to the DEEP_RESEARCH prompt, for example after the file coordination section:
- Use file summaries to track progress, surface blockers, and keep an audit trail for the final synthesis. +**Collect structured user input when needed:** +- Use 'render_webapp' to create interactive forms for collecting structured data from users (email addresses, preferences, survey responses, etc.) +- Use 'get_webapp_data' to retrieve submitted form data +- Use 'remove_webapp' to clean up the interface after data collection + **Clear instructions to research agents must include:**front_end/panels/ai_chat/tools/Tools.ts (1)
3506-3523: Verify whether new tools should be added to getTools() function.The new webapp and file management tools are exported from the module but do not appear to be added to the
getTools()function (lines 3447-3502). This function is documented as "Returns all available tools" and is likely used by other parts of the codebase to discover tools.Should these new tools be added to the
getTools()function, or are they intentionally omitted (e.g., because they're only meant to be used by specific agents that instantiate them directly)?Also, the comment on line 3507 says "Export HTML injection tools" but these tools render full webapps in iframes rather than injecting HTML. Consider updating to:
-// Export HTML injection tools +// Export webapp rendering toolsfront_end/panels/ai_chat/agent_framework/implementation/agents/ContentWriterAgent.ts (1)
28-33: Strengthen coordination guidance with canonical filename + JSONL appendAdd a short note recommending a canonical coordination filename (e.g., content_writer/<objective_slug>.jsonl) and JSON Lines with update_file append:true to avoid collisions and enable incremental, auditable appends.
front_end/panels/ai_chat/tools/UpdateFileTool.ts (2)
29-50: Add JSON Schema default for append to guide callersSet a default for append to reduce malformed calls and make intent explicit.
append: { type: 'boolean', - description: 'Whether to append the content instead of replacing it (default: false)' + description: 'Whether to append the content instead of replacing it', + default: false },
52-71: Return fileName (and size) for parity with create/read toolsInclude fileName (and size) so downstream steps can chain without re-reading.
- return { - success: true, - fileId: file.id, - message: `${action} file "${file.fileName}" (${file.size} bytes).` - }; + return { + success: true, + fileId: file.id, + fileName: file.fileName, + size: file.size, + message: `${action} file "${file.fileName}" (${file.size} bytes).` + };front_end/panels/ai_chat/agent_framework/implementation/agents/SearchAgent.ts (1)
26-26: Standardize coordination file pattern + JSONL appendRecommend documenting a canonical coordination filename (e.g., search/<objective_slug>.jsonl) and JSON Lines entries with timestamps. Always use update_file append:true to avoid overwrites and enable multi-agent audit trails.
Also applies to: 36-36, 63-63
front_end/panels/ai_chat/tools/ListFilesTool.ts (2)
26-35: Drop required reasoning (unused) to reduce call frictionexecute() doesn’t use reasoning. Make it optional to avoid unnecessary tool call failures.
schema = { type: 'object', properties: { reasoning: { type: 'string', description: 'Explanation for why the file list is needed' } }, - required: ['reasoning'] + // reasoning is optional; left for traceability if the caller provides it };
41-47: Consider adding totalSize to the resultSumming files[i].size provides a quick overview without another pass. Optional DX improvement.
- return { - success: true, - files, - count: files.length - }; + const totalSize = files.reduce((n, f) => n + (f.size || 0), 0); + return { success: true, files, count: files.length, totalSize };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
front_end/panels/ai_chat/BUILD.gn(2 hunks)front_end/panels/ai_chat/agent_framework/implementation/ConfiguredAgents.ts(2 hunks)front_end/panels/ai_chat/agent_framework/implementation/agents/ActionAgent.ts(1 hunks)front_end/panels/ai_chat/agent_framework/implementation/agents/ContentWriterAgent.ts(2 hunks)front_end/panels/ai_chat/agent_framework/implementation/agents/ResearchAgent.ts(4 hunks)front_end/panels/ai_chat/agent_framework/implementation/agents/SearchAgent.ts(5 hunks)front_end/panels/ai_chat/agent_framework/implementation/agents/WebTaskAgent.ts(1 hunks)front_end/panels/ai_chat/core/BaseOrchestratorAgent.ts(6 hunks)front_end/panels/ai_chat/tools/CreateFileTool.ts(1 hunks)front_end/panels/ai_chat/tools/DeleteFileTool.ts(1 hunks)front_end/panels/ai_chat/tools/FileStorageManager.ts(1 hunks)front_end/panels/ai_chat/tools/GetWebAppDataTool.ts(1 hunks)front_end/panels/ai_chat/tools/ListFilesTool.ts(1 hunks)front_end/panels/ai_chat/tools/ReadFileTool.ts(1 hunks)front_end/panels/ai_chat/tools/RemoveWebAppTool.ts(1 hunks)front_end/panels/ai_chat/tools/RenderWebAppTool.ts(1 hunks)front_end/panels/ai_chat/tools/Tools.ts(2 hunks)front_end/panels/ai_chat/tools/UpdateFileTool.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (11)
front_end/panels/ai_chat/tools/DeleteFileTool.ts (3)
front_end/panels/ai_chat/core/Logger.ts (1)
createLogger(316-318)front_end/panels/ai_chat/tools/Tools.ts (2)
Tool(43-52)LLMContext(57-65)front_end/panels/ai_chat/tools/FileStorageManager.ts (1)
FileStorageManager(44-281)
front_end/panels/ai_chat/agent_framework/implementation/ConfiguredAgents.ts (9)
front_end/panels/ai_chat/agent_framework/ConfigurableAgentTool.ts (1)
ToolRegistry(189-236)front_end/panels/ai_chat/tools/CreateFileTool.ts (1)
CreateFileTool(26-73)front_end/panels/ai_chat/tools/UpdateFileTool.ts (1)
UpdateFileTool(25-72)front_end/panels/ai_chat/tools/DeleteFileTool.ts (1)
DeleteFileTool(22-59)front_end/panels/ai_chat/tools/ReadFileTool.ts (1)
ReadFileTool(27-76)front_end/panels/ai_chat/tools/ListFilesTool.ts (1)
ListFilesTool(22-56)front_end/panels/ai_chat/tools/RenderWebAppTool.ts (1)
RenderWebAppTool(35-240)front_end/panels/ai_chat/tools/GetWebAppDataTool.ts (1)
GetWebAppDataTool(35-261)front_end/panels/ai_chat/tools/RemoveWebAppTool.ts (1)
RemoveWebAppTool(32-131)
front_end/panels/ai_chat/tools/GetWebAppDataTool.ts (2)
front_end/panels/ai_chat/core/Logger.ts (1)
createLogger(316-318)front_end/panels/ai_chat/tools/Tools.ts (3)
Tool(43-52)ErrorResult(113-115)LLMContext(57-65)
front_end/panels/ai_chat/tools/ReadFileTool.ts (3)
front_end/panels/ai_chat/core/Logger.ts (1)
createLogger(316-318)front_end/panels/ai_chat/tools/Tools.ts (2)
Tool(43-52)LLMContext(57-65)front_end/panels/ai_chat/tools/FileStorageManager.ts (2)
FileStorageManager(44-281)StoredFile(17-26)
front_end/panels/ai_chat/tools/RenderWebAppTool.ts (2)
front_end/panels/ai_chat/core/Logger.ts (1)
createLogger(316-318)front_end/panels/ai_chat/tools/Tools.ts (3)
Tool(43-52)ErrorResult(113-115)LLMContext(57-65)
front_end/panels/ai_chat/tools/CreateFileTool.ts (3)
front_end/panels/ai_chat/core/Logger.ts (1)
createLogger(316-318)front_end/panels/ai_chat/tools/Tools.ts (2)
Tool(43-52)LLMContext(57-65)front_end/panels/ai_chat/tools/FileStorageManager.ts (2)
FileStorageManager(44-281)StoredFile(17-26)
front_end/panels/ai_chat/tools/FileStorageManager.ts (1)
front_end/panels/ai_chat/core/Logger.ts (1)
createLogger(316-318)
front_end/panels/ai_chat/tools/UpdateFileTool.ts (3)
front_end/panels/ai_chat/core/Logger.ts (1)
createLogger(316-318)front_end/panels/ai_chat/tools/Tools.ts (2)
Tool(43-52)LLMContext(57-65)front_end/panels/ai_chat/tools/FileStorageManager.ts (2)
FileStorageManager(44-281)StoredFile(17-26)
front_end/panels/ai_chat/tools/RemoveWebAppTool.ts (2)
front_end/panels/ai_chat/core/Logger.ts (1)
createLogger(316-318)front_end/panels/ai_chat/tools/Tools.ts (3)
Tool(43-52)ErrorResult(113-115)LLMContext(57-65)
front_end/panels/ai_chat/tools/ListFilesTool.ts (3)
front_end/panels/ai_chat/core/Logger.ts (1)
createLogger(316-318)front_end/panels/ai_chat/tools/Tools.ts (2)
Tool(43-52)LLMContext(57-65)front_end/panels/ai_chat/tools/FileStorageManager.ts (2)
FileSummary(28-34)FileStorageManager(44-281)
front_end/panels/ai_chat/core/BaseOrchestratorAgent.ts (8)
front_end/panels/ai_chat/tools/RenderWebAppTool.ts (1)
RenderWebAppTool(35-240)front_end/panels/ai_chat/tools/GetWebAppDataTool.ts (1)
GetWebAppDataTool(35-261)front_end/panels/ai_chat/tools/RemoveWebAppTool.ts (1)
RemoveWebAppTool(32-131)front_end/panels/ai_chat/tools/CreateFileTool.ts (1)
CreateFileTool(26-73)front_end/panels/ai_chat/tools/UpdateFileTool.ts (1)
UpdateFileTool(25-72)front_end/panels/ai_chat/tools/DeleteFileTool.ts (1)
DeleteFileTool(22-59)front_end/panels/ai_chat/tools/ReadFileTool.ts (1)
ReadFileTool(27-76)front_end/panels/ai_chat/tools/ListFilesTool.ts (1)
ListFilesTool(22-56)
🔇 Additional comments (7)
front_end/panels/ai_chat/agent_framework/implementation/agents/WebTaskAgent.ts (1)
207-214: Verify tool registration and permissions for new tool namesEnsure these tool names are registered in the tool factory/registry and granted to this agent; otherwise runtime tool-call failures will occur.
front_end/panels/ai_chat/agent_framework/implementation/agents/ActionAgent.ts (1)
94-101: New Action Agent tools are registered and available. All tool names in createActionAgentConfig match implementations in front_end/panels/ai_chat/tools.front_end/panels/ai_chat/agent_framework/implementation/ConfiguredAgents.ts (1)
11-61: Tool registrations look solid.Thanks for wiring the new file + webapp tools straight into the registry; this keeps agent capability surfaces in sync with the prompt updates.
front_end/panels/ai_chat/core/BaseOrchestratorAgent.ts (1)
535-539: Verify intentional omission of webapp tools from default toolset.The default toolset in
getAgentToolsincludes the new file management tools but omits the webapp tools (RenderWebAppTool,GetWebAppDataTool,RemoveWebAppTool), while the specific SEARCH and DEEP_RESEARCH agent configurations include both sets of tools (lines 318-326, 342-349).Is this intentional, or should the default toolset also include webapp tools for consistency? If webapp functionality is only meant for specific agent types, consider documenting this design decision.
front_end/panels/ai_chat/BUILD.gn (1)
94-104: LGTM!The new tool source files are correctly added to the build manifest in both the
devtools_modulesources list and the_ai_chat_sourceslist. The additions are consistent and properly formatted.front_end/panels/ai_chat/agent_framework/implementation/agents/SearchAgent.ts (1)
135-141: Verify tool availability across exports and registrationsDouble-check that these tools are exported and registered so the agent can invoke them.
Run:
front_end/panels/ai_chat/agent_framework/implementation/agents/ContentWriterAgent.ts (1)
58-64: Confirm exports and runtime registration for file tools
All five tools (read_file,list_files,create_file,update_file,delete_file) are properly exported in Tools.ts; double-check your orchestrator or agent configuration includes them in its tools list so they resolve at runtime.
| if (inputType === 'checkbox') { | ||
| // For checkboxes, check if we already have this field name | ||
| if (fieldName in formData) { | ||
| // Multiple checkboxes with same name - create array | ||
| if (!Array.isArray(formData[fieldName])) { | ||
| formData[fieldName] = [formData[fieldName]]; | ||
| } | ||
| if (element.checked) { | ||
| formData[fieldName].push(element.value || true); | ||
| } | ||
| } else { | ||
| // First checkbox with this name | ||
| formData[fieldName] = element.checked ? (element.value || true) : false; | ||
| } | ||
| } else if (inputType === 'radio') { | ||
| // For radio buttons, only store if checked | ||
| if (element.checked) { | ||
| formData[fieldName] = element.value || true; | ||
| } | ||
| } else { | ||
| // Text, email, number, password, etc. | ||
| formData[fieldName] = element.value || ''; | ||
| } | ||
| } else if (element.tagName.toLowerCase() === 'select') { | ||
| // For select elements, get selected value | ||
| if (element.multiple) { | ||
| // Multiple select - get array of selected values | ||
| const selectedOptions = Array.from(element.selectedOptions || []); | ||
| formData[fieldName] = selectedOptions.map(opt => opt.value); | ||
| } else { | ||
| // Single select | ||
| formData[fieldName] = element.value || ''; | ||
| } | ||
| } else if (element.tagName.toLowerCase() === 'textarea') { | ||
| // For textareas | ||
| formData[fieldName] = element.value || ''; | ||
| } | ||
| }); | ||
| return { |
There was a problem hiding this comment.
Checkbox aggregation returns corrupt data.
When multiple checkboxes share a name, the current logic seeds the entry with false for unchecked boxes, so a later checked box produces arrays like [false, 'option']. Callers then see bogus false members instead of just the checked values. Please only store checked values and add an explicit false when none were selected.
- const inputs = iframeDoc.querySelectorAll('input, select, textarea');
-
- inputs.forEach((element) => {
+ const inputs = iframeDoc.querySelectorAll('input, select, textarea');
+ const checkboxFields = new Set();
+
+ inputs.forEach((element) => {
// Get the field identifier (prefer name, fallback to id)
const fieldName = element.name || element.id;
// Skip elements without identifiers
if (!fieldName) {
return;
}
// Extract value based on element type
if (element.tagName.toLowerCase() === 'input') {
const inputType = element.type ? element.type.toLowerCase() : 'text';
if (inputType === 'checkbox') {
- // For checkboxes, check if we already have this field name
- if (fieldName in formData) {
- // Multiple checkboxes with same name - create array
- if (!Array.isArray(formData[fieldName])) {
- formData[fieldName] = [formData[fieldName]];
- }
- if (element.checked) {
- formData[fieldName].push(element.value || true);
- }
- } else {
- // First checkbox with this name
- formData[fieldName] = element.checked ? (element.value || true) : false;
- }
+ checkboxFields.add(fieldName);
+ if (!element.checked) {
+ return;
+ }
+ if (fieldName in formData) {
+ if (!Array.isArray(formData[fieldName])) {
+ formData[fieldName] = [formData[fieldName]];
+ }
+ formData[fieldName].push(element.value || true);
+ } else {
+ formData[fieldName] = element.value || true;
+ }
+ return;
} else if (inputType === 'radio') {
// For radio buttons, only store if checked
if (element.checked) {
formData[fieldName] = element.value || true;
}
@@
});
+
+ checkboxFields.forEach((name) => {
+ if (!(name in formData)) {
+ formData[name] = false;
+ }
+ });📝 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.
| if (inputType === 'checkbox') { | |
| // For checkboxes, check if we already have this field name | |
| if (fieldName in formData) { | |
| // Multiple checkboxes with same name - create array | |
| if (!Array.isArray(formData[fieldName])) { | |
| formData[fieldName] = [formData[fieldName]]; | |
| } | |
| if (element.checked) { | |
| formData[fieldName].push(element.value || true); | |
| } | |
| } else { | |
| // First checkbox with this name | |
| formData[fieldName] = element.checked ? (element.value || true) : false; | |
| } | |
| } else if (inputType === 'radio') { | |
| // For radio buttons, only store if checked | |
| if (element.checked) { | |
| formData[fieldName] = element.value || true; | |
| } | |
| } else { | |
| // Text, email, number, password, etc. | |
| formData[fieldName] = element.value || ''; | |
| } | |
| } else if (element.tagName.toLowerCase() === 'select') { | |
| // For select elements, get selected value | |
| if (element.multiple) { | |
| // Multiple select - get array of selected values | |
| const selectedOptions = Array.from(element.selectedOptions || []); | |
| formData[fieldName] = selectedOptions.map(opt => opt.value); | |
| } else { | |
| // Single select | |
| formData[fieldName] = element.value || ''; | |
| } | |
| } else if (element.tagName.toLowerCase() === 'textarea') { | |
| // For textareas | |
| formData[fieldName] = element.value || ''; | |
| } | |
| }); | |
| return { | |
| const inputs = iframeDoc.querySelectorAll('input, select, textarea'); | |
| const checkboxFields = new Set(); | |
| inputs.forEach((element) => { | |
| // Get the field identifier (prefer name, fallback to id) | |
| const fieldName = element.name || element.id; | |
| // Skip elements without identifiers | |
| if (!fieldName) { | |
| return; | |
| } | |
| // Extract value based on element type | |
| if (element.tagName.toLowerCase() === 'input') { | |
| const inputType = element.type ? element.type.toLowerCase() : 'text'; | |
| if (inputType === 'checkbox') { | |
| checkboxFields.add(fieldName); | |
| // Only record checked boxes | |
| if (!element.checked) { | |
| return; | |
| } | |
| if (fieldName in formData) { | |
| if (!Array.isArray(formData[fieldName])) { | |
| formData[fieldName] = [formData[fieldName]]; | |
| } | |
| formData[fieldName].push(element.value || true); | |
| } else { | |
| formData[fieldName] = element.value || true; | |
| } | |
| return; | |
| } else if (inputType === 'radio') { | |
| // For radio buttons, only store if checked | |
| if (element.checked) { | |
| formData[fieldName] = element.value || true; | |
| } | |
| } else { | |
| // Text, email, number, password, etc. | |
| formData[fieldName] = element.value || ''; | |
| } | |
| } else if (element.tagName.toLowerCase() === 'select') { | |
| // For select elements, get selected value | |
| if (element.multiple) { | |
| // Multiple select - get array of selected values | |
| const selectedOptions = Array.from(element.selectedOptions || []); | |
| formData[fieldName] = selectedOptions.map(opt => opt.value); | |
| } else { | |
| // Single select | |
| formData[fieldName] = element.value || ''; | |
| } | |
| } else if (element.tagName.toLowerCase() === 'textarea') { | |
| // For textareas | |
| formData[fieldName] = element.value || ''; | |
| } | |
| }); | |
| // If no checked boxes were recorded for a checkbox field, set it to false | |
| checkboxFields.forEach((name) => { | |
| if (!(name in formData)) { | |
| formData[name] = false; | |
| } | |
| }); | |
| return { |
| const iframe = document.createElement('iframe'); | ||
| iframe.id = webappId; | ||
| iframe.setAttribute('data-devtools-webapp', 'true'); | ||
| iframe.setAttribute('data-reasoning', ${JSON.stringify(reasoning)}); | ||
| // Style iframe for full-screen coverage | ||
| iframe.style.position = 'fixed'; | ||
| iframe.style.top = '0'; | ||
| iframe.style.left = '0'; | ||
| iframe.style.width = '100vw'; | ||
| iframe.style.height = '100vh'; | ||
| iframe.style.border = 'none'; | ||
| iframe.style.zIndex = '999999'; | ||
| iframe.style.backgroundColor = 'white'; | ||
| // Build complete HTML document for iframe | ||
| const fullHTML = '<!DOCTYPE html>' + | ||
| '<html>' + | ||
| '<head>' + | ||
| '<meta charset="UTF-8">' + | ||
| '<meta name="viewport" content="width=device-width, initial-scale=1.0">' + | ||
| (${JSON.stringify(css || '')} ? '<style>' + ${JSON.stringify(css || '')} + '</style>' : '') + | ||
| '</head>' + | ||
| '<body>' + | ||
| ${JSON.stringify(html)} + | ||
| '</body>' + | ||
| '</html>'; | ||
| // Set iframe content using srcdoc | ||
| iframe.srcdoc = fullHTML; | ||
| // Append iframe to body | ||
| document.body.appendChild(iframe); | ||
There was a problem hiding this comment.
Add the missing iframe sandbox to enforce isolation.
The tool promises a sandboxed iframe, but we never set the sandbox attribute. As written, the generated iframe inherits the parent’s origin, so untrusted webapp code can reach back into DevTools (window.parent, DOM mutation, navigation), defeating the isolation guarantee. Please set an explicit sandbox policy (while still allowing scripts/forms so downstream tools keep working).
const iframe = document.createElement('iframe');
iframe.id = webappId;
iframe.setAttribute('data-devtools-webapp', 'true');
iframe.setAttribute('data-reasoning', ${JSON.stringify(reasoning)});
+ iframe.setAttribute('sandbox', 'allow-scripts allow-same-origin allow-forms');📝 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 iframe = document.createElement('iframe'); | |
| iframe.id = webappId; | |
| iframe.setAttribute('data-devtools-webapp', 'true'); | |
| iframe.setAttribute('data-reasoning', ${JSON.stringify(reasoning)}); | |
| // Style iframe for full-screen coverage | |
| iframe.style.position = 'fixed'; | |
| iframe.style.top = '0'; | |
| iframe.style.left = '0'; | |
| iframe.style.width = '100vw'; | |
| iframe.style.height = '100vh'; | |
| iframe.style.border = 'none'; | |
| iframe.style.zIndex = '999999'; | |
| iframe.style.backgroundColor = 'white'; | |
| // Build complete HTML document for iframe | |
| const fullHTML = '<!DOCTYPE html>' + | |
| '<html>' + | |
| '<head>' + | |
| '<meta charset="UTF-8">' + | |
| '<meta name="viewport" content="width=device-width, initial-scale=1.0">' + | |
| (${JSON.stringify(css || '')} ? '<style>' + ${JSON.stringify(css || '')} + '</style>' : '') + | |
| '</head>' + | |
| '<body>' + | |
| ${JSON.stringify(html)} + | |
| '</body>' + | |
| '</html>'; | |
| // Set iframe content using srcdoc | |
| iframe.srcdoc = fullHTML; | |
| // Append iframe to body | |
| document.body.appendChild(iframe); | |
| const iframe = document.createElement('iframe'); | |
| iframe.id = webappId; | |
| iframe.setAttribute('data-devtools-webapp', 'true'); | |
| iframe.setAttribute('data-reasoning', ${JSON.stringify(reasoning)}); | |
| iframe.setAttribute('sandbox', 'allow-scripts allow-same-origin allow-forms'); | |
| // Style iframe for full-screen coverage | |
| iframe.style.position = 'fixed'; | |
| iframe.style.top = '0'; | |
| iframe.style.left = '0'; | |
| iframe.style.width = '100vw'; | |
| iframe.style.height = '100vh'; | |
| iframe.style.border = 'none'; | |
| iframe.style.zIndex = '999999'; | |
| iframe.style.backgroundColor = 'white'; | |
| // Build complete HTML document for iframe | |
| const fullHTML = '<!DOCTYPE html>' + | |
| '<html>' + | |
| '<head>' + | |
| '<meta charset="UTF-8">' + | |
| '<meta name="viewport" content="width=device-width, initial-scale=1.0">' + | |
| (${JSON.stringify(css || '')} ? '<style>' + ${JSON.stringify(css || '')} + '</style>' : '') + | |
| '</head>' + | |
| '<body>' + | |
| ${JSON.stringify(html)} + | |
| '</body>' + | |
| '</html>'; | |
| // Set iframe content using srcdoc | |
| iframe.srcdoc = fullHTML; | |
| // Append iframe to body | |
| document.body.appendChild(iframe); |
🤖 Prompt for AI Agents
In front_end/panels/ai_chat/tools/RenderWebAppTool.ts around lines 94 to 127,
the iframe is created without a sandbox attribute which breaks the promised
isolation; add an explicit sandbox policy that still permits scripts and forms
but blocks access to the parent/origin by setting iframe.setAttribute('sandbox',
'allow-scripts allow-forms'); place this immediately after creating the iframe
(before setting srcdoc and appending to document.body).
|
Claude finished @tysonthomas9's task —— View job PR Review - Web Asset Rendering and Virtual File SystemReviewing the pull request for code quality, bugs, performance, security, and test coverage. Todo List:
Overall AssessmentThis PR adds a comprehensive virtual file system and web application rendering capabilities to the AI chat interface. The implementation includes 8 new tools and updates to several agents. Here's my detailed review: ✅ Strengths1. Clean Architecture & Design
2. Robust Error Handling
3. Security Considerations
|
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (5)
front_end/panels/ai_chat/ui/ChatView.ts (3)
1188-1218: Add UX state and minimal arg hygiene for re-render
- Disable the “Re-render” button while running; re-enable on completion/error.
- Optionally pass only needed fields (e.g., html/css/js) to avoid leaking extraneous args.
- Surface user notifications (success/error) instead of only logs.
1176-1186: Avoid direct DOM show/hide; prefer reactive stateToggling
style.displaycan desync with Lit re-renders. Track expanded state in component state and render conditionally.
580-600: Use shared constant for tool ID Import and use a centralized tool name constant (e.g.RENDER_WEBAPP_TOOL_NAME) from your tools module or registry instead of hard-coding'render_webapp'.front_end/panels/ai_chat/ui/ToolResultComponent.ts (1)
18-18: Consider stronger typing for toolCallArgs.The
Record<string, any>type provides minimal type safety. If the structure of tool call arguments is known or can be constrained, consider defining a more specific interface or using a union of known tool argument types.front_end/panels/ai_chat/ui/__tests__/WebAppCodeViewer.test.ts (1)
10-10: Testing private methods reduces encapsulation.Multiple tests access private methods using
(WebAppCodeViewer as any). Consider either:
- Making these methods public if they need testing
- Testing only through the public API (
showmethod)- Extracting testable logic into separate helper functions
Testing through the public API is generally preferred as it tests the component's actual contract and is resilient to refactoring.
Also applies to: 64-64, 181-181, 236-236
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
config/gni/devtools_grd_files.gni(3 hunks)front_end/panels/ai_chat/BUILD.gn(6 hunks)front_end/panels/ai_chat/tools/__tests__/CreateFileTool.test.ts(1 hunks)front_end/panels/ai_chat/tools/__tests__/FileStorageManager.test.ts(1 hunks)front_end/panels/ai_chat/tools/__tests__/ListFilesTool.test.ts(1 hunks)front_end/panels/ai_chat/tools/__tests__/ReadFileTool.test.ts(1 hunks)front_end/panels/ai_chat/tools/__tests__/UpdateFileTool.test.ts(1 hunks)front_end/panels/ai_chat/ui/ChatView.ts(3 hunks)front_end/panels/ai_chat/ui/ToolResultComponent.ts(4 hunks)front_end/panels/ai_chat/ui/WebAppCodeViewer.ts(1 hunks)front_end/panels/ai_chat/ui/__tests__/WebAppCodeViewer.test.ts(1 hunks)front_end/panels/ai_chat/ui/chatView.css(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- config/gni/devtools_grd_files.gni
🧰 Additional context used
🧬 Code graph analysis (9)
front_end/panels/ai_chat/tools/__tests__/FileStorageManager.test.ts (1)
front_end/panels/ai_chat/tools/FileStorageManager.ts (3)
FileStorageManager(44-281)FileSummary(28-34)StoredFile(17-26)
front_end/panels/ai_chat/tools/__tests__/ReadFileTool.test.ts (1)
front_end/panels/ai_chat/tools/FileStorageManager.ts (2)
StoredFile(17-26)FileStorageManager(44-281)
front_end/panels/ai_chat/ui/__tests__/WebAppCodeViewer.test.ts (2)
front_end/panels/ai_chat/ui/WebAppCodeViewer.ts (1)
WebAppCodeViewer(18-701)front_end/panels/ai_chat/ui/ChatView.ts (2)
toolArgs(1191-1218)toolArgs(1223-1230)
front_end/panels/ai_chat/ui/ToolResultComponent.ts (1)
front_end/panels/ai_chat/agent_framework/AgentSessionTypes.ts (1)
AgentMessage(46-53)
front_end/panels/ai_chat/ui/ChatView.ts (2)
front_end/panels/ai_chat/core/Logger.ts (1)
error(254-303)front_end/panels/ai_chat/ui/WebAppCodeViewer.ts (1)
WebAppCodeViewer(18-701)
front_end/panels/ai_chat/ui/WebAppCodeViewer.ts (2)
front_end/panels/ai_chat/core/Logger.ts (1)
createLogger(316-318)front_end/panels/ai_chat/ui/ChatView.ts (2)
toolArgs(1191-1218)toolArgs(1223-1230)
front_end/panels/ai_chat/tools/__tests__/CreateFileTool.test.ts (1)
front_end/panels/ai_chat/tools/FileStorageManager.ts (2)
StoredFile(17-26)FileStorageManager(44-281)
front_end/panels/ai_chat/tools/__tests__/ListFilesTool.test.ts (1)
front_end/panels/ai_chat/tools/FileStorageManager.ts (2)
FileSummary(28-34)FileStorageManager(44-281)
front_end/panels/ai_chat/tools/__tests__/UpdateFileTool.test.ts (1)
front_end/panels/ai_chat/tools/FileStorageManager.ts (2)
StoredFile(17-26)FileStorageManager(44-281)
🔇 Additional comments (14)
front_end/panels/ai_chat/tools/__tests__/ReadFileTool.test.ts (1)
28-43: Good coverage and clean stubbingCovers success, not-found, and error paths; singleton is safely stubbed and restored.
front_end/panels/ai_chat/tools/__tests__/ListFilesTool.test.ts (2)
13-29: LGTM on success pathAsserts returned data and count; verifies single manager call. Clean pattern with restore in afterEach.
31-40: LGTM on error pathError is propagated as expected; good negative coverage.
front_end/panels/ai_chat/tools/__tests__/UpdateFileTool.test.ts (2)
28-59: Well-covered replace and append pathsValidates message text and correct manager invocation (append flag). Clean test helpers and restore.
61-70: LGTM on failure pathError propagation asserted correctly.
front_end/panels/ai_chat/tools/__tests__/FileStorageManager.test.ts (1)
8-11: <!-- [scratchpad]
[task review_file_2/2]
[observations]
- Tests reset singleton via cast on private static.
- No
resetForTestinginFileStorageManager.ts.- No “fake-indexeddb” or polyfill import found in tests.
[analysis]- Directly mutating private static is brittle.
- Tests may fail without an IndexedDB polyfill in Node.
[pending]- Confirm test runner’s IndexedDB support.
[actions]- Add
static resetForTesting()to FileStorageManager.- Replace cast in beforeEach with
FileStorageManager.resetForTesting().- Import a polyfill (e.g.
fake-indexeddb/auto) in test setup or add to this file.
](###hidden)Add test-only reset API and polyfill IndexedDB in tests
In FileStorageManager.ts add:// Test-only static resetForTesting(): void { FileStorageManager.instance = null; }In your test’s beforeEach replace the cast with:
FileStorageManager.resetForTesting();And ensure IndexedDB is available in Node (e.g. install and import
fake-indexeddb/autoin your Jest setup).front_end/panels/ai_chat/ui/ToolResultComponent.ts (2)
25-29: Verify if toolCallArgs requires validation.The method accepts and stores arbitrary
toolCallArgswithout validation. Ensure that the upstream caller provides sanitized data, especially if these arguments will be re-dispatched via events (as inhandleReRender).
112-141: LGTM! Well-structured UI for re-render functionality.The CSS and conditional rendering logic are well-designed:
- Proper use of CSS custom properties for theming
- Hover and active states provide good user feedback
stopPropagationcorrectly prevents triggering the parent's expand/collapse- Button includes
titleattribute for accessibilityAlso applies to: 224-240
front_end/panels/ai_chat/ui/__tests__/WebAppCodeViewer.test.ts (2)
7-54: Comprehensive test coverage for escapeHTML.The test suite thoroughly covers HTML escaping scenarios including edge cases like double-escaping, empty strings, and special character combinations. This is critical for preventing XSS vulnerabilities.
318-357: Integration tests handle environment limitations appropriately.The tests acknowledge that
show()will fail in the test environment and handle errors gracefully. This is acceptable for integration-level tests when full infrastructure isn't available.front_end/panels/ai_chat/ui/chatView.css (3)
2198-2240: Well-structured styling for integrated tool results.The CSS creates a cohesive visual language for tool results within the timeline:
- Proper use of CSS custom properties for theming
- Status-based border colors provide clear visual feedback
- Subtle pulse animation for running state is performance-friendly
- Hover states improve interactivity
2302-2370: Enhanced JSON result presentation with good UX.The JSON result styling provides:
- Monospace font for code readability
- Proper scrolling with max-height constraint
- Custom scrollbar styling for visual consistency
- Clear visual separation from surrounding content
2241-2300: Verify WCAG AA contrast for action buttons Ensure.re-render-webapp-button(bg: rgba(var(--sys-color-primary-rgb), 0.1), text: var(--sys-color-primary)) and.webapp-action-btn(bg: rgba(var(--sys-color-tertiary-rgb), 0.1), text: var(--sys-color-tertiary)) meet at least 4.5:1 contrast ratio.front_end/panels/ai_chat/BUILD.gn (1)
45-45: LGTM! Proper build configuration for new files.All new source files are correctly added to:
devtools_module("ai_chat")sources_ai_chat_sourceslist for metadata generationts_library("unittests")for test filesThe additions are consistent with the existing file organization and naming conventions.
Also applies to: 95-105, 210-210, 262-272, 416-441
| sinon.assert.calledOnce((fakeManager as any).createFile); | ||
| sinon.assert.calledWithExactly((fakeManager as any).createFile, 'note.txt', 'hello', undefined); | ||
| }); |
There was a problem hiding this comment.
Loosen stub assertion to avoid brittleness on optional arg
Enforcing a third arg of undefined will fail if the implementation calls with only two args or supplies a default mime type. Match the required args instead.
Apply this diff:
- sinon.assert.calledWithExactly((fakeManager as any).createFile, 'note.txt', 'hello', undefined);
+ sinon.assert.calledWith((fakeManager as any).createFile, 'note.txt', 'hello');📝 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.
| sinon.assert.calledOnce((fakeManager as any).createFile); | |
| sinon.assert.calledWithExactly((fakeManager as any).createFile, 'note.txt', 'hello', undefined); | |
| }); | |
| sinon.assert.calledOnce((fakeManager as any).createFile); | |
| sinon.assert.calledWith((fakeManager as any).createFile, 'note.txt', 'hello'); | |
| }); |
🤖 Prompt for AI Agents
In front_end/panels/ai_chat/tools/__tests__/CreateFileTool.test.ts around lines
37 to 39, the test currently asserts the third argument is exactly undefined
which is brittle; update the assertion to only match the required args by
removing the strict check for the third parameter — use
sinon.assert.calledWithExactly((fakeManager as any).createFile, 'note.txt',
'hello') or sinon.assert.calledWith((fakeManager as any).createFile, 'note.txt',
'hello') so the test passes whether the implementation omits the third arg or
provides a default mime type.
| private handleReRender(): void { | ||
| if (!this.toolCallArgs) return; | ||
|
|
||
| const event = new CustomEvent('re-render-webapp', { | ||
| detail: { | ||
| toolName: 'render_webapp', | ||
| toolArgs: this.toolCallArgs | ||
| }, | ||
| bubbles: true, | ||
| composed: true | ||
| }); | ||
|
|
||
| this.dispatchEvent(event); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Verify event listener exists for 're-render-webapp'.
The method dispatches a custom event 're-render-webapp' that bubbles through the component tree. Ensure that a listener for this event exists in a parent component (likely ChatView or similar).
🏁 Script executed:
#!/bin/bash
# Search for event listeners that handle 're-render-webapp'
rg -n "re-render-webapp" --type ts -C3Length of output: 1334
Missing listener for 're-render-webapp'. Ensure ChatView.ts listens for the dispatched event, e.g.:
<tool-result-component
…
@re-render-webapp=${this.#handleReRenderWebApp}
></tool-result-component>| <button class="download-all-btn" onclick="downloadAll(event)"> | ||
| <svg viewBox="0 0 24 24" fill="none" stroke="currentColor" stroke-width="2"> | ||
| <path d="M21 15v4a2 2 0 0 1-2 2H5a2 2 0 0 1-2-2v-4"></path> | ||
| <polyline points="7 10 12 15 17 10"></polyline> | ||
| <line x1="12" y1="15" x2="12" y2="3"></line> | ||
| </svg> | ||
| Download All | ||
| </button> |
There was a problem hiding this comment.
Inline event handlers and global exposures violate CSP; attach listeners in JS
Inline onclick and window.* globals will be blocked under strict CSP and are harder to maintain. Remove inline handlers and wire events via addEventListener on load.
Apply these diffs to remove inline handlers and add stable hooks:
- <button class="download-all-btn" onclick="downloadAll(event)">
+ <button class="download-all-btn" id="download-all-btn">- <button class="copy-btn" onclick="copyHTML(event)" data-code-type="html">
+ <button class="copy-btn" data-code-type="html">- <button class="download-btn" onclick="downloadHTML(event)" data-code-type="html">
+ <button class="download-btn" data-code-type="html">- <button class="copy-btn" onclick="copyCSS(event)" data-code-type="css">
+ <button class="copy-btn" data-code-type="css">- <button class="download-btn" onclick="downloadCSS(event)" data-code-type="css">
+ <button class="download-btn" data-code-type="css">- <button class="copy-btn" onclick="copyJS(event)" data-code-type="js">
+ <button class="copy-btn" data-code-type="js">- <button class="download-btn" onclick="downloadJS(event)" data-code-type="js">
+ <button class="download-btn" data-code-type="js">And replace the global exposures with DOM event wiring (near the end of buildJS output):
-// Expose handlers globally for inline onclick attributes
-// This ensures buttons like onclick="downloadHTML(event)" can resolve.
-window.copyHTML = copyHTML;
-window.copyCSS = copyCSS;
-window.copyJS = copyJS;
-window.downloadHTML = downloadHTML;
-window.downloadCSS = downloadCSS;
-window.downloadJS = downloadJS;
-window.downloadAll = downloadAll;
+// Wire up event listeners once DOM is ready
+document.addEventListener('DOMContentLoaded', () => {
+ document.querySelectorAll('.copy-btn').forEach(btn => {
+ btn.addEventListener('click', (event) => {
+ const type = (btn as HTMLElement).getAttribute('data-code-type') || '';
+ const codeEl = document.getElementById(`${type}-code`);
+ const code = codeEl?.textContent || '';
+ copyToClipboard(code, (event.target as HTMLElement)?.closest('.copy-btn'));
+ });
+ });
+ document.querySelectorAll('.download-btn').forEach(btn => {
+ btn.addEventListener('click', (event) => {
+ const type = (btn as HTMLElement).getAttribute('data-code-type') || '';
+ const codeEl = document.getElementById(`${type}-code`);
+ const code = codeEl?.textContent || '';
+ const filename = type === 'html' ? 'index.html' : (type === 'css' ? 'styles.css' : 'script.js');
+ downloadFile(code, filename, (event.target as HTMLElement)?.closest('.download-btn'));
+ });
+ });
+ document.getElementById('download-all-btn')?.addEventListener('click', (event) => {
+ downloadAll(event);
+ });
+});Also applies to: 94-109, 124-139, 154-169, 679-687
| const originalText = button.textContent; | ||
| button.textContent = 'Downloading...'; | ||
| // Download files sequentially with small delays | ||
| downloadHTML(); | ||
| setTimeout(() => downloadCSS(), 100); | ||
| setTimeout(() => downloadJS(), 200); | ||
| // Reset button after downloads | ||
| setTimeout(() => { | ||
| button.textContent = '✓ Downloaded!'; | ||
| setTimeout(() => { | ||
| button.textContent = originalText; | ||
| }, 2000); | ||
| }, 300); |
There was a problem hiding this comment.
Download-all button loses SVG icon after first click
Using textContent replaces and drops the SVG permanently. Preserve and restore innerHTML.
- const originalText = button.textContent;
- button.textContent = 'Downloading...';
+ const originalHTML = button.innerHTML;
+ button.innerHTML = 'Downloading...';
@@
- button.textContent = '✓ Downloaded!';
+ button.innerHTML = '✓ Downloaded!';
setTimeout(() => {
- button.textContent = originalText;
+ button.innerHTML = originalHTML;
}, 2000);📝 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 originalText = button.textContent; | |
| button.textContent = 'Downloading...'; | |
| // Download files sequentially with small delays | |
| downloadHTML(); | |
| setTimeout(() => downloadCSS(), 100); | |
| setTimeout(() => downloadJS(), 200); | |
| // Reset button after downloads | |
| setTimeout(() => { | |
| button.textContent = '✓ Downloaded!'; | |
| setTimeout(() => { | |
| button.textContent = originalText; | |
| }, 2000); | |
| }, 300); | |
| const originalHTML = button.innerHTML; | |
| button.innerHTML = 'Downloading...'; | |
| // Download files sequentially with small delays | |
| downloadHTML(); | |
| setTimeout(() => downloadCSS(), 100); | |
| setTimeout(() => downloadJS(), 200); | |
| // Reset button after downloads | |
| setTimeout(() => { | |
| button.innerHTML = '✓ Downloaded!'; | |
| setTimeout(() => { | |
| button.innerHTML = originalHTML; | |
| }, 2000); | |
| }, 300); |
🤖 Prompt for AI Agents
In front_end/panels/ai_chat/ui/WebAppCodeViewer.ts around lines 613 to 627, the
button textContent is being replaced which removes the SVG icon; capture and
reuse the button.innerHTML instead of textContent so the SVG markup is
preserved. Save const originalInnerHTML = button.innerHTML before changing it,
set button.innerHTML = 'Downloading...' and later restore button.innerHTML = '✓
Downloaded!' and finally reset to originalInnerHTML; keep the same timing logic
but operate on innerHTML to avoid dropping the SVG.
Summary by CodeRabbit