put fix/explain with copilot directly in the status bar (#233927)

* put copilot fix and explain in status bar

* fix up actions

* watch for execution error within the viewmodel

* make observable publicly readonly

* remove unused service

* fix tests

* remove unused import
This commit is contained in:
Aaron Munger
2024-11-15 11:36:12 -08:00
committed by GitHub
parent 10c9715567
commit b8ad8f5101
11 changed files with 211 additions and 67 deletions
@@ -1700,6 +1700,7 @@ export function createApiFactoryAndRegisterActors(accessor: ServicesAccessor): I
NotebookEditorRevealType: extHostTypes.NotebookEditorRevealType,
NotebookCellOutput: extHostTypes.NotebookCellOutput,
NotebookCellOutputItem: extHostTypes.NotebookCellOutputItem,
CellErrorStackFrame: extHostTypes.CellErrorStackFrame,
NotebookCellStatusBarItem: extHostTypes.NotebookCellStatusBarItem,
NotebookControllerAffinity: extHostTypes.NotebookControllerAffinity,
NotebookControllerAffinity2: extHostTypes.NotebookControllerAffinity2,
@@ -721,17 +721,7 @@ class NotebookCellExecutionTask extends Disposable {
// so we use updateSoon and immediately flush.
that._collector.flush();
const error = executionError ? {
message: executionError.message,
stack: executionError.stack,
location: executionError?.location ? {
startLineNumber: executionError.location.start.line,
startColumn: executionError.location.start.character,
endLineNumber: executionError.location.end.line,
endColumn: executionError.location.end.character
} : undefined,
uri: executionError.uri
} : undefined;
const error = createSerializeableError(executionError);
that._proxy.$completeExecution(that._handle, new SerializableObjectWithBuffers({
runEndTime: endTime,
@@ -769,6 +759,31 @@ class NotebookCellExecutionTask extends Disposable {
}
}
function createSerializeableError(executionError: vscode.CellExecutionError | undefined) {
const convertRange = (range: vscode.Range | undefined) => (range ? {
startLineNumber: range.start.line,
startColumn: range.start.character,
endLineNumber: range.end.line,
endColumn: range.end.character
} : undefined);
const convertStackFrame = (frame: vscode.CellErrorStackFrame) => ({
uri: frame.uri,
position: frame.position,
label: frame.label
});
const error = executionError ? {
name: executionError.name,
message: executionError.message,
stack: executionError.stack instanceof Array
? executionError.stack.map(frame => convertStackFrame(frame))
: executionError.stack,
location: convertRange(executionError.location),
uri: executionError.uri
} : undefined;
return error;
}
enum NotebookExecutionTaskState {
Init,
@@ -3907,6 +3907,19 @@ export class NotebookCellOutput {
}
}
export class CellErrorStackFrame {
/**
* @param label The name of the stack frame
* @param file The file URI of the stack frame
* @param position The position of the stack frame within the file
*/
constructor(
public label: string,
public uri?: vscode.Uri,
public position?: Position,
) { }
}
export enum NotebookCellKind {
Markup = 1,
Code = 2
@@ -6,30 +6,23 @@
import { Disposable, IDisposable, toDisposable } from '../../../../../../base/common/lifecycle.js';
import { IMarkerData, IMarkerService } from '../../../../../../platform/markers/common/markers.js';
import { IRange } from '../../../../../../editor/common/core/range.js';
import { ICellExecutionError, ICellExecutionStateChangedEvent, IExecutionStateChangedEvent, INotebookExecutionStateService, NotebookExecutionType } from '../../../common/notebookExecutionStateService.js';
import { ICellExecutionStateChangedEvent, IExecutionStateChangedEvent, INotebookExecutionStateService, NotebookExecutionType } from '../../../common/notebookExecutionStateService.js';
import { IConfigurationService } from '../../../../../../platform/configuration/common/configuration.js';
import { CellKind, NotebookSetting } from '../../../common/notebookCommon.js';
import { INotebookEditor, INotebookEditorContribution } from '../../notebookBrowser.js';
import { registerNotebookContribution } from '../../notebookEditorExtensions.js';
import { Iterable } from '../../../../../../base/common/iterator.js';
import { CodeCellViewModel } from '../../viewModel/codeCellViewModel.js';
import { URI } from '../../../../../../base/common/uri.js';
import { Event } from '../../../../../../base/common/event.js';
import { IChatAgentService } from '../../../../chat/common/chatAgents.js';
type CellDiagnostic = {
cellUri: URI;
error: ICellExecutionError;
disposables: IDisposable[];
};
export class CellDiagnostics extends Disposable implements INotebookEditorContribution {
static ID: string = 'workbench.notebook.cellDiagnostics';
private enabled = false;
private listening = false;
private diagnosticsByHandle: Map<number, CellDiagnostic> = new Map();
private diagnosticsByHandle: Map<number, IDisposable[]> = new Map();
constructor(
private readonly notebookEditor: INotebookEditor,
@@ -66,8 +59,6 @@ export class CellDiagnostics extends Disposable implements INotebookEditorContri
}
}
private handleChangeExecutionState(changes: (ICellExecutionStateChangedEvent | IExecutionStateChangedEvent)[]) {
if (!this.enabled) {
return;
@@ -96,9 +87,9 @@ export class CellDiagnostics extends Disposable implements INotebookEditorContri
}
public clear(cellHandle: number) {
const diagnostic = this.diagnosticsByHandle.get(cellHandle);
if (diagnostic) {
for (const disposable of diagnostic.disposables) {
const disposables = this.diagnosticsByHandle.get(cellHandle);
if (disposables) {
for (const disposable of disposables) {
disposable.dispose();
}
this.diagnosticsByHandle.delete(cellHandle);
@@ -119,11 +110,10 @@ export class CellDiagnostics extends Disposable implements INotebookEditorContri
const metadata = cell.model.internalMetadata;
if (cell instanceof CodeCellViewModel && !metadata.lastRunSuccess && metadata?.error?.location) {
const disposables: IDisposable[] = [];
const marker = this.createMarkerData(metadata.error.message, metadata.error.location);
const errorLabel = metadata.error.name ? `${metadata.error.name}: ${metadata.error.message}` : metadata.error.message;
const marker = this.createMarkerData(errorLabel, metadata.error.location);
this.markerService.changeOne(CellDiagnostics.ID, cell.uri, [marker]);
disposables.push(toDisposable(() => this.markerService.changeOne(CellDiagnostics.ID, cell.uri, [])));
cell.excecutionError.set(metadata.error, undefined);
disposables.push(toDisposable(() => cell.excecutionError.set(undefined, undefined)));
disposables.push(cell.model.onDidChangeOutputs(() => {
if (cell.model.outputs.length === 0) {
this.clear(cellHandle);
@@ -132,7 +122,7 @@ export class CellDiagnostics extends Disposable implements INotebookEditorContri
disposables.push(cell.model.onDidChangeContent(() => {
this.clear(cellHandle);
}));
this.diagnosticsByHandle.set(cellHandle, { cellUri: cell.uri, error: metadata.error, disposables });
this.diagnosticsByHandle.set(cellHandle, disposables);
}
}
@@ -15,8 +15,13 @@ import { KeybindingWeight } from '../../../../../../platform/keybinding/common/k
import { INotebookCellActionContext, NotebookCellAction, findTargetCellEditor } from '../../controller/coreActions.js';
import { CodeCellViewModel } from '../../viewModel/codeCellViewModel.js';
import { NOTEBOOK_CELL_EDITOR_FOCUSED, NOTEBOOK_CELL_FOCUSED, NOTEBOOK_CELL_HAS_ERROR_DIAGNOSTICS } from '../../../common/notebookContextKeys.js';
import { InlineChatController } from '../../../../inlineChat/browser/inlineChatController.js';
import { showChatView } from '../../../../chat/browser/chat.js';
import { IViewsService } from '../../../../../services/views/common/viewsService.js';
export const OPEN_CELL_FAILURE_ACTIONS_COMMAND_ID = 'notebook.cell.openFailureActions';
export const FIX_CELL_ERROR_COMMAND_ID = 'notebook.cell.chat.fixError';
export const EXPLAIN_CELL_ERROR_COMMAND_ID = 'notebook.cell.chat.explainError';
registerAction2(class extends NotebookCellAction {
constructor() {
@@ -35,7 +40,7 @@ registerAction2(class extends NotebookCellAction {
async runWithContext(accessor: ServicesAccessor, context: INotebookCellActionContext): Promise<void> {
if (context.cell instanceof CodeCellViewModel) {
const error = context.cell.excecutionError.get();
const error = context.cell.executionError.get();
if (error?.location) {
const location = Range.lift({
startLineNumber: error.location.startLineNumber + 1,
@@ -56,3 +61,61 @@ registerAction2(class extends NotebookCellAction {
}
}
});
registerAction2(class extends NotebookCellAction {
constructor() {
super({
id: FIX_CELL_ERROR_COMMAND_ID,
title: localize2('notebookActions.chatFixCellError', "Fix Cell Error"),
precondition: ContextKeyExpr.and(NOTEBOOK_CELL_FOCUSED, NOTEBOOK_CELL_HAS_ERROR_DIAGNOSTICS, NOTEBOOK_CELL_EDITOR_FOCUSED.toNegated()),
f1: true
});
}
async runWithContext(accessor: ServicesAccessor, context: INotebookCellActionContext): Promise<void> {
if (context.cell instanceof CodeCellViewModel) {
const error = context.cell.executionError.get();
if (error?.location) {
const location = Range.lift({
startLineNumber: error.location.startLineNumber + 1,
startColumn: error.location.startColumn + 1,
endLineNumber: error.location.endLineNumber + 1,
endColumn: error.location.endColumn + 1
});
context.notebookEditor.setCellEditorSelection(context.cell, Range.lift(location));
const editor = findTargetCellEditor(context, context.cell);
if (editor) {
const controller = InlineChatController.get(editor);
const message = error.name ? `${error.name}: ${error.message}` : error.message;
if (controller) {
await controller.run({ message: '/fix ' + message, initialRange: location, autoSend: true });
}
}
}
}
}
});
registerAction2(class extends NotebookCellAction {
constructor() {
super({
id: EXPLAIN_CELL_ERROR_COMMAND_ID,
title: localize2('notebookActions.chatExplainCellError', "Explain Cell Error"),
precondition: ContextKeyExpr.and(NOTEBOOK_CELL_FOCUSED, NOTEBOOK_CELL_HAS_ERROR_DIAGNOSTICS, NOTEBOOK_CELL_EDITOR_FOCUSED.toNegated()),
f1: true
});
}
async runWithContext(accessor: ServicesAccessor, context: INotebookCellActionContext): Promise<void> {
if (context.cell instanceof CodeCellViewModel) {
const error = context.cell.executionError.get();
if (error?.message) {
const viewsService = accessor.get(IViewsService);
const chatWidget = await showChatView(viewsService);
const message = error.name ? `${error.name}: ${error.message}` : error.message;
// TODO: can we add special prompt instructions? e.g. use "%pip install"
chatWidget?.acceptInput('@workspace /explain ' + message,);
}
}
}
});
@@ -7,8 +7,7 @@ import { Disposable } from '../../../../../../base/common/lifecycle.js';
import { autorun } from '../../../../../../base/common/observable.js';
import { localize } from '../../../../../../nls.js';
import { IInstantiationService } from '../../../../../../platform/instantiation/common/instantiation.js';
import { IKeybindingService } from '../../../../../../platform/keybinding/common/keybinding.js';
import { OPEN_CELL_FAILURE_ACTIONS_COMMAND_ID } from './cellDiagnosticsActions.js';
import { EXPLAIN_CELL_ERROR_COMMAND_ID, FIX_CELL_ERROR_COMMAND_ID } from './cellDiagnosticsActions.js';
import { NotebookStatusBarController } from '../cellStatusBar/executionStatusBarItemController.js';
import { INotebookEditor, INotebookEditorContribution, INotebookViewModel } from '../../notebookBrowser.js';
import { registerNotebookContribution } from '../../notebookEditorExtensions.js';
@@ -39,31 +38,31 @@ class DiagnosticCellStatusBarItem extends Disposable {
constructor(
private readonly _notebookViewModel: INotebookViewModel,
private readonly cell: CodeCellViewModel,
@IKeybindingService private readonly keybindingService: IKeybindingService,
private readonly cell: CodeCellViewModel
) {
super();
this._register(autorun((reader) => this.updateSparkleItem(reader.readObservable(cell.excecutionError))));
this._register(autorun((reader) => this.updateQuickActions(reader.readObservable(cell.executionError))));
}
private async updateSparkleItem(error: ICellExecutionError | undefined) {
let item: INotebookCellStatusBarItem | undefined;
private async updateQuickActions(error: ICellExecutionError | undefined) {
let items: INotebookCellStatusBarItem[] = [];
if (error?.location) {
const keybinding = this.keybindingService.lookupKeybinding(OPEN_CELL_FAILURE_ACTIONS_COMMAND_ID)?.getLabel();
const tooltip = localize('notebook.cell.status.diagnostic', "Quick Actions {0}", `(${keybinding})`);
item = {
text: `$(sparkle)`,
tooltip,
items = [{
text: `$(sparkle) fix`,
tooltip: localize('notebook.cell.status.fix', 'Fix With Inline Chat'),
alignment: CellStatusbarAlignment.Left,
command: OPEN_CELL_FAILURE_ACTIONS_COMMAND_ID,
command: FIX_CELL_ERROR_COMMAND_ID,
priority: Number.MAX_SAFE_INTEGER - 1
};
}, {
text: `$(sparkle) explain`,
tooltip: localize('notebook.cell.status.explain', 'Explain With Chat'),
alignment: CellStatusbarAlignment.Left,
command: EXPLAIN_CELL_ERROR_COMMAND_ID,
priority: Number.MAX_SAFE_INTEGER - 1
}];
}
const items = item ? [item] : [];
this._currentItemIds = this._notebookViewModel.deltaCellStatusBarItems(this._currentItemIds, [{ handle: this.cell.handle, items }]);
}
@@ -103,7 +103,7 @@ export class CellContextKeyManager extends Disposable {
if (element instanceof CodeCellViewModel) {
this.elementDisposables.add(element.onDidChangeOutputs(() => this.updateForOutputs()));
this.elementDisposables.add(autorun(reader => {
this.cellHasErrorDiagnostics.set(!!reader.readObservable(element.excecutionError));
this.cellHasErrorDiagnostics.set(!!reader.readObservable(element.executionError));
}));
}
@@ -5,14 +5,13 @@
import { Emitter, Event, PauseableEmitter } from '../../../../../base/common/event.js';
import { dispose } from '../../../../../base/common/lifecycle.js';
import { observableValue } from '../../../../../base/common/observable.js';
import { IObservable, observableValue } from '../../../../../base/common/observable.js';
import * as UUID from '../../../../../base/common/uuid.js';
import { ICodeEditorService } from '../../../../../editor/browser/services/codeEditorService.js';
import * as editorCommon from '../../../../../editor/common/editorCommon.js';
import { PrefixSumComputer } from '../../../../../editor/common/model/prefixSumComputer.js';
import { ITextModelService } from '../../../../../editor/common/services/resolverService.js';
import { IConfigurationService } from '../../../../../platform/configuration/common/configuration.js';
import { IInstantiationService } from '../../../../../platform/instantiation/common/instantiation.js';
import { IUndoRedoService } from '../../../../../platform/undoRedo/common/undoRedo.js';
import { CellEditState, CellFindMatch, CellLayoutState, CodeCellLayoutChangeEvent, CodeCellLayoutInfo, ICellOutputViewModel, ICellViewModel } from '../notebookBrowser.js';
import { NotebookOptionsChangeEvent } from '../notebookOptions.js';
@@ -135,7 +134,11 @@ export class CodeCellViewModel extends BaseCellViewModel implements ICellViewMod
return this._outputViewModels;
}
readonly excecutionError = observableValue<ICellExecutionError | undefined>('excecutionError', undefined);
get executionError(): IObservable<ICellExecutionError | undefined> {
return this._executionError;
}
private readonly _executionError = observableValue<ICellExecutionError | undefined>('excecutionError', undefined);
constructor(
viewType: string,
@@ -146,8 +149,7 @@ export class CodeCellViewModel extends BaseCellViewModel implements ICellViewMod
@INotebookService private readonly _notebookService: INotebookService,
@ITextModelService modelService: ITextModelService,
@IUndoRedoService undoRedoService: IUndoRedoService,
@ICodeEditorService codeEditorService: ICodeEditorService,
@IInstantiationService instantiationService: IInstantiationService
@ICodeEditorService codeEditorService: ICodeEditorService
) {
super(viewType, model, UUID.generateUuid(), viewContext, configurationService, modelService, undoRedoService, codeEditorService);
this._outputViewModels = this.model.outputs.map(output => new CellOutputViewModel(this, output, this._notebookService));
@@ -170,6 +172,9 @@ export class CodeCellViewModel extends BaseCellViewModel implements ICellViewMod
if (outputLayoutChange) {
this.layoutChange({ outputHeight: true }, 'CodeCellViewModel#model.onDidChangeOutputs');
}
if (!this._outputCollection.length) {
this._executionError.set(undefined, undefined);
}
dispose(removedOutputs);
}));
@@ -200,9 +205,14 @@ export class CodeCellViewModel extends BaseCellViewModel implements ICellViewMod
updateExecutionState(e: ICellExecutionStateChangedEvent) {
if (e.changed) {
this._executionError.set(undefined, undefined);
this._onDidStartExecution.fire(e);
} else {
this._onDidStopExecution.fire(e);
if (this.internalMetadata.lastRunSuccess === false && this.internalMetadata.error) {
const metadata = this.internalMetadata;
this._executionError.set(metadata.error, undefined);
}
}
}
@@ -21,12 +21,29 @@ export interface ICellExecutionStateUpdate {
isPaused?: boolean;
}
export interface ICellErrorStackFrame {
/**
* The location of this stack frame. This should be provided as a URI if the
* location of the call frame can be accessed by the editor.
*/
readonly uri?: UriComponents;
readonly location?: IRange;
/**
* The name of the stack frame, typically a method or function name.
*/
readonly label: string;
}
export interface ICellExecutionError {
name: string;
message: string;
stack: string | undefined;
stack: string | ICellErrorStackFrame[] | undefined;
uri: UriComponents;
location: IRange | undefined;
}
export interface ICellExecutionComplete {
runEndTime?: number;
lastRunSuccess?: boolean;
@@ -7,7 +7,6 @@ import assert from 'assert';
import { Emitter, Event } from '../../../../../../base/common/event.js';
import { DisposableStore } from '../../../../../../base/common/lifecycle.js';
import { ResourceMap } from '../../../../../../base/common/map.js';
import { waitForState } from '../../../../../../base/common/observable.js';
import { URI } from '../../../../../../base/common/uri.js';
import { mock } from '../../../../../../base/test/common/mock.js';
import { ensureNoDisposablesAreLeakedInTestSuite } from '../../../../../../base/test/common/utils.js';
@@ -55,6 +54,7 @@ suite('notebookCellDiagnostics', () => {
interface ITestMarkerService extends IMarkerService {
markers: ResourceMap<IMarkerData[]>;
onMarkersUpdated: Event<void>;
}
setup(function () {
@@ -88,9 +88,12 @@ suite('notebookCellDiagnostics', () => {
instantiationService.stub(IChatAgentService, chatAgentService);
markerService = new class extends mock<ITestMarkerService>() {
private _onMarkersUpdated = new Emitter<void>();
override onMarkersUpdated = this._onMarkersUpdated.event;
override markers: ResourceMap<IMarkerData[]> = new ResourceMap();
override changeOne(owner: string, resource: URI, markers: IMarkerData[]) {
this.markers.set(resource, markers);
this._onMarkersUpdated.fire();
}
};
instantiationService.stub(IMarkerService, markerService);
@@ -107,16 +110,18 @@ suite('notebookCellDiagnostics', () => {
disposables.add(instantiationService.createInstance(CellDiagnostics, editor));
cell.model.internalMetadata.lastRunSuccess = false;
cell.model.internalMetadata.error = {
message: 'error',
name: 'error',
message: 'something bad happened',
stack: 'line 1 : print(x)',
uri: cell.uri,
location: { startColumn: 1, endColumn: 5, startLineNumber: 1, endLineNumber: 1 }
};
testExecutionService.fireExecutionChanged(editor.textModel.uri, cell.handle);
await new Promise<void>(resolve => Event.once(markerService.onMarkersUpdated)(resolve));
await waitForState(cell.excecutionError, error => !!error);
assert.strictEqual(cell?.excecutionError.get()?.message, 'error');
assert.strictEqual(cell?.executionError.get()?.message, 'something bad happened');
assert.equal(markerService.markers.get(cell.uri)?.length, 1);
}, instantiationService);
});
@@ -131,14 +136,18 @@ suite('notebookCellDiagnostics', () => {
disposables.add(instantiationService.createInstance(CellDiagnostics, editor));
cell.model.internalMetadata.lastRunSuccess = false;
cell.model.internalMetadata.error = {
message: 'error',
name: 'error',
message: 'something bad happened',
stack: 'line 1 : print(x)',
uri: cell.uri,
location: { startColumn: 1, endColumn: 5, startLineNumber: 1, endLineNumber: 1 }
};
cell2.model.internalMetadata.lastRunSuccess = false;
cell2.model.internalMetadata.error = {
message: 'another error',
name: 'error',
message: 'another bad thing happened',
stack: 'line 1 : print(y)',
uri: cell.uri,
location: { startColumn: 1, endColumn: 5, startLineNumber: 1, endLineNumber: 1 }
@@ -146,17 +155,16 @@ suite('notebookCellDiagnostics', () => {
testExecutionService.fireExecutionChanged(editor.textModel.uri, cell.handle);
testExecutionService.fireExecutionChanged(editor.textModel.uri, cell2.handle);
await waitForState(cell.excecutionError, error => !!error);
await waitForState(cell2.excecutionError, error => !!error);
await new Promise<void>(resolve => Event.once(markerService.onMarkersUpdated)(resolve));
cell.model.internalMetadata.error = undefined;
// on NotebookCellExecution value will make it look like its currently running
testExecutionService.fireExecutionChanged(editor.textModel.uri, cell.handle, {} as INotebookCellExecution);
await waitForState(cell.excecutionError, error => error === undefined);
await new Promise<void>(resolve => Event.once(markerService.onMarkersUpdated)(resolve));
assert.strictEqual(cell?.excecutionError.get(), undefined);
assert.strictEqual(cell2?.excecutionError.get()?.message, 'another error', 'cell that was not executed should still have an error');
assert.strictEqual(cell?.executionError.get(), undefined);
assert.strictEqual(cell2?.executionError.get()?.message, 'another bad thing happened', 'cell that was not executed should still have an error');
assert.equal(markerService.markers.get(cell.uri)?.length, 0);
assert.equal(markerService.markers.get(cell2.uri)?.length, 1);
}, instantiationService);
+30 -2
View File
@@ -19,15 +19,20 @@ declare module 'vscode' {
}
export interface CellExecutionError {
/**
* The error name.
*/
readonly name: string;
/**
* The error message.
*/
readonly message: string;
/**
* The error stack trace.
* The string from an Error object or parsed details on each stack frame to help with diagnostics.
*/
readonly stack: string | undefined;
readonly stack: string | CellErrorStackFrame[] | undefined;
/**
* The cell resource which had the error.
@@ -38,7 +43,30 @@ declare module 'vscode' {
* The location within the resource where the error occurred.
*/
readonly location: Range | undefined;
}
export class CellErrorStackFrame {
/**
* The location of this stack frame. This should be provided as a URI if the
* location of the call frame can be accessed by the editor.
*/
readonly uri?: Uri;
/**
* Position of the stack frame within the file.
*/
position?: Position;
/**
* The name of the stack frame, typically a method or function name.
*/
readonly label: string;
/**
* @param label The name of the stack frame
* @param file The file URI of the stack frame
* @param position The position of the stack frame within the file
*/
constructor(label: string, uri?: Uri, position?: Position);
}
}