simple browser: console logs to chat - #277293
Conversation
📬 CODENOTIFYThe following users are being notified based on files changed in this PR: Johannes Rieken (@jrieken)Matched files:
|
There was a problem hiding this comment.
Pull Request Overview
This PR adds functionality to capture and attach console logs from the Simple Browser to chat conversations. It implements console log collection using Chrome DevTools Protocol in Electron and provides a UI button to attach these logs to the chat context.
Key Changes
- Added console log collection via Chrome DevTools Protocol during debug sessions
- Created a new
getConsoleLogs()method across the service layer (platform → workbench) - Added an "Attach Logs" button (bug icon) in the Simple Browser overlay UI
- Logs are stored in a module-level Map with a maximum of 5 entries per window
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
| src/vs/platform/browserElements/electron-main/nativeBrowserElementsMainService.ts | Implements console log collection via CDP, stores logs in a Map, and formats them for display |
| src/vs/platform/browserElements/common/browserElements.ts | Adds getConsoleLogs() method to INativeBrowserElementsService interface |
| src/vs/workbench/services/browserElements/electron-browser/browserElementsService.ts | Implements workbench-level wrapper for getConsoleLogs() |
| src/vs/workbench/services/browserElements/browser/browserElementsService.ts | Adds getConsoleLogs() to IBrowserElementsService interface |
| src/vs/workbench/services/browserElements/browser/webBrowserElementsService.ts | Adds stub implementation throwing "not implemented" error for web |
| src/vs/workbench/contrib/chat/browser/chatEditing/simpleBrowserEditorOverlay.ts | Adds "Attach Logs" button and addConsolesToChat() method to attach logs to chat |
| src/vs/workbench/contrib/chat/browser/media/simpleBrowserOverlay.css | Adds CSS styling for the new bug icon button |
Comments suppressed due to low confidence (1)
src/vs/platform/browserElements/electron-main/nativeBrowserElementsMainService.ts:318
- The
allConsoleMap is accessed without any synchronization mechanism. Since theonMessagehandler can add entries to the Map concurrently withgetConsoleLogsreading from it, there's a potential race condition. While JavaScript is single-threaded, consider using a snapshot of the logs or ensuring proper handling if logs are being modified during formatting.
const logs = allConsole.get(window.id) ?? [];
if (logs.length === 0) {
return undefined;
}
const formatted = logs.map(log => {
let line = '';
if (log.type === 'console') {
line = `[${log.level?.toUpperCase()}] ${log.message}`;
} else if (log.type === 'exception') {
line = `[ERROR] Exception: ${log.message}`;
if (log.exceptionDetails?.stackTrace?.callFrames?.length) {
line += '\nStack Trace:';
log.exceptionDetails.stackTrace.callFrames.forEach((frame: any) => {
line += `\n at ${frame.functionName} (${frame.url}:${frame.lineNumber}:${frame.columnNumber})`;
});
}
} else if (log.type === 'log') {
line = `[${log.level?.toUpperCase()}] ${log.message}`;
}
return line;
}).join('\n');
return formatted;
}
async finishOverlay(debuggers: any, sessionId: string | undefined): Promise<void> {
if (debuggers.isAttached() && sessionId) {
await debuggers.sendCommand('Overlay.setInspectMode', {
| const onMessage = (event: any, method: string, params: any, sessionIdFromMessage?: string) => { | ||
| if (sessionIdFromMessage === sessionId && (method === 'Runtime.consoleAPICalled' || method === 'Runtime.exceptionThrown' || method === 'Log.entryAdded')) { | ||
| const current = allConsole.get(windowId!) ?? []; | ||
|
|
||
| let logEntry: LogEntry; | ||
|
|
||
| if (method === 'Runtime.consoleAPICalled') { | ||
| // Extract console message from args | ||
| const args = params.args?.map((arg: any) => { | ||
| if (arg.type === 'string') { | ||
| return arg.value; | ||
| } | ||
| if (arg.type === 'object') { | ||
| // For objects, try to extract a readable representation | ||
| if (arg.preview?.description) { | ||
| return arg.preview.description; | ||
| } | ||
| return JSON.stringify(arg, null, 2); | ||
| } | ||
| if (arg.value !== undefined) { | ||
| return arg.value; | ||
| } | ||
| return JSON.stringify(arg, null, 2); | ||
| }) ?? []; | ||
|
|
||
| logEntry = { | ||
| type: 'console', | ||
| level: params.type, // 'log', 'debug', 'info', 'error', 'warning', 'dir', 'dirxml', 'table', 'trace', 'clear', 'startGroup', 'startGroupCollapsed', 'endGroup' | ||
| message: args.join(' '), | ||
| args: params.args, | ||
| timestamp: Date.now(), | ||
| rawData: JSON.stringify(params) | ||
| }; | ||
| } else if (method === 'Runtime.exceptionThrown') { | ||
| const exceptionDetails = params.exceptionDetails; | ||
| let errorMessage = exceptionDetails?.text || 'Unknown error'; | ||
| if (exceptionDetails?.exception?.description) { | ||
| errorMessage = exceptionDetails.exception.description; | ||
| } | ||
|
|
||
| logEntry = { | ||
| type: 'exception', | ||
| level: 'error', | ||
| message: errorMessage, | ||
| exceptionDetails: params.exceptionDetails, | ||
| timestamp: Date.now(), | ||
| rawData: JSON.stringify(params) | ||
| }; | ||
| } else { // Log.entryAdded | ||
| logEntry = { | ||
| type: 'log', | ||
| level: params.level, // 'verbose', 'info', 'warning', 'error' | ||
| message: params.text, | ||
| timestamp: Date.now(), | ||
| rawData: JSON.stringify(params) | ||
| }; | ||
| } | ||
|
|
||
| if (!current.some(entry => entry.rawData === logEntry.rawData)) { | ||
| current.push(logEntry); | ||
| if (current.length > 5) { | ||
| current.shift(); | ||
| } | ||
| allConsole.set(windowId!, current); | ||
| } | ||
| } | ||
| }; | ||
|
|
||
| try { |
There was a problem hiding this comment.
The onMessage event handler captures windowId from the outer scope, but windowId could potentially be undefined (as the parameter type indicates). While it's checked at line 140, using the non-null assertion (windowId!) at lines 160, 226 could fail if the check at 140 doesn't return. Consider checking windowId is defined before setting up the message handler or using a local constant to store the validated windowId.
| } | ||
|
|
||
| async getConsoleLogs(): Promise<string | undefined> { | ||
| return await this.simpleBrowser.getConsoleLogs(); |
There was a problem hiding this comment.
[nitpick] The await keyword is unnecessary here since the method is directly returning the promise from this.simpleBrowser.getConsoleLogs(). This can be simplified to return this.simpleBrowser.getConsoleLogs(); for cleaner code.
| return await this.simpleBrowser.getConsoleLogs(); | |
| return this.simpleBrowser.getConsoleLogs(); |
| targetId: matchingTargetId, | ||
| flatten: true, | ||
| }); | ||
| sessionId = attachResult.sessionId; |
There was a problem hiding this comment.
This comment does not accurately describe what's happening. The code already attached to the target and enabled console logging above (lines 241-250). This section is for handling detachment on IPC messages, not "starts console logging".
| rawData: string; | ||
| } | ||
|
|
||
| const allConsole = new Map<number, LogEntry[]>(); |
There was a problem hiding this comment.
The global module-level Map (allConsole) storing console logs has no cleanup mechanism. As users open/close windows, this Map will continue to grow indefinitely, leading to a memory leak. Consider adding cleanup logic when windows are closed (e.g., in a window close event handler) to remove entries from the Map.
| async addConsolesToChat() { | ||
| const logs = await this._browserElementsService.getConsoleLogs(); | ||
| const toAttach: IChatRequestVariableEntry[] = []; | ||
|
|
||
| toAttach.push({ | ||
| id: 'element-' + Date.now(), | ||
| name: localize('consoleLogs', 'Console Logs'), | ||
| fullName: localize('consoleLogs', 'Console Logs'), | ||
| value: logs, | ||
| kind: 'element', | ||
| icon: ThemeIcon.fromId(Codicon.bug.id), | ||
| }); | ||
|
|
||
| const widget = await showChatView(this._viewService, this._layoutService) ?? this._chatWidgetService.lastFocusedWidget; | ||
| widget?.attachmentModel?.addContext(...toAttach); | ||
| } |
There was a problem hiding this comment.
The addConsolesToChat method doesn't handle the case where logs could be undefined. If no logs are available, the method will attach a context entry with value: undefined, which may not be the desired behavior. Consider adding a check and either showing a message to the user or returning early if no logs are available.
| const cancelButtonLabel = localize('cancelSelectionLabel', 'Cancel'); | ||
| cancelButton.label = cancelButtonLabel; | ||
|
|
||
| const attachLogs = this._showStore.add(new Button(mainContent, { supportIcons: true, title: localize('chat.attachLogs', "Attach Logs") })); |
There was a problem hiding this comment.
The attachLogs button is missing the defaultButtonStyles spread that other buttons in this file use (e.g., cancelButton, configure). This inconsistency may lead to different visual appearance. Consider adding ...defaultButtonStyles to maintain consistency with other buttons.
| const attachLogs = this._showStore.add(new Button(mainContent, { supportIcons: true, title: localize('chat.attachLogs', "Attach Logs") })); | |
| const attachLogs = this._showStore.add(new Button(mainContent, { ...defaultButtonStyles, supportIcons: true, title: localize('chat.attachLogs', "Attach Logs") })); |
| message?: string; | ||
| args?: any[]; | ||
| exceptionDetails?: any; | ||
| params?: any; |
There was a problem hiding this comment.
The params property in the LogEntry interface appears to be unused. It's only assigned in the interface definition but never used elsewhere in the code, and in the console API logging, the args field already stores the relevant data.
| params?: any; |
| } | ||
|
|
||
| let sessionId: string | undefined; | ||
| const onMessage = (event: any, method: string, params: any, sessionIdFromMessage?: string) => { |
There was a problem hiding this comment.
Using any type for debuggers parameter lacks type safety. Consider defining a proper interface or type for the debugger object to improve code maintainability and catch potential errors at compile time.
| const onMessage = (event: any, method: string, params: any, sessionIdFromMessage?: string) => { | |
| /** | |
| * Types for debugger protocol messages | |
| */ | |
| interface ConsoleAPICalledParams { | |
| args?: Array<{ | |
| type: string; | |
| value?: unknown; | |
| preview?: { | |
| description?: string; | |
| }; | |
| }>; | |
| type: string; | |
| } | |
| interface ExceptionThrownParams { | |
| exceptionDetails?: { | |
| text?: string; | |
| exception?: { | |
| description?: string; | |
| }; | |
| }; | |
| } | |
| interface LogEntryAddedParams { | |
| level: string; | |
| text: string; | |
| } | |
| type DebuggerParams = ConsoleAPICalledParams | ExceptionThrownParams | LogEntryAddedParams; | |
| const onMessage = (event: unknown, method: string, params: DebuggerParams, sessionIdFromMessage?: string) => { |
| const args = params.args?.map((arg: any) => { | ||
| if (arg.type === 'string') { | ||
| return arg.value; | ||
| } | ||
| if (arg.type === 'object') { | ||
| // For objects, try to extract a readable representation | ||
| if (arg.preview?.description) { | ||
| return arg.preview.description; | ||
| } | ||
| return JSON.stringify(arg, null, 2); | ||
| } | ||
| if (arg.value !== undefined) { | ||
| return arg.value; | ||
| } | ||
| return JSON.stringify(arg, null, 2); | ||
| }) ?? []; |
There was a problem hiding this comment.
Multiple any types used (arg: any, frame: any) reduce type safety. Consider defining proper interfaces for the Chrome DevTools Protocol message structure (e.g., RemoteObject, CallFrame) to improve code maintainability.
| if (activeBrowserType) { | ||
| try { | ||
| await this._browserElementsService.startDebugSession(cts.token, activeBrowserType); | ||
|
|
There was a problem hiding this comment.
There's an extra blank line after the startDebugSession call. Remove this blank line to maintain consistency with the codebase style.
a rehash of this PR #248459 which has been floating around forever. lots of chrome dev tools work to get logging working!
cc Kyle Cutler (@kycutler) since I think you're taking over some of the simple browser work
Screen.Recording.2025-11-13.at.3.19.10.PM.mov