From 7114851c4a4b7102ac287c43bd9d4e54a55eb787 Mon Sep 17 00:00:00 2001 From: Rob Lourens Date: Tue, 7 Mar 2023 22:30:46 -0500 Subject: [PATCH] Add errorDetails to interactive session responses. (#176457) * Add errorDetails to interactive session responses. Also avoid calling updateElementHeight during a renderElement * Fix hygiene --- .../workbench/api/common/extHost.protocol.ts | 3 +- .../api/common/extHostInteractiveSession.ts | 29 +++++--- .../browser/interactiveSessionListRenderer.ts | 73 +++++++++++-------- .../browser/media/interactiveSession.css | 10 +++ .../common/interactiveSessionModel.ts | 33 ++++++--- .../common/interactiveSessionService.ts | 3 +- .../common/interactiveSessionServiceImpl.ts | 8 +- .../common/interactiveSessionViewModel.ts | 9 ++- .../vscode.proposed.interactive.d.ts | 6 ++ 9 files changed, 118 insertions(+), 56 deletions(-) diff --git a/src/vs/workbench/api/common/extHost.protocol.ts b/src/vs/workbench/api/common/extHost.protocol.ts index c95ef3058ef..94f349d9e16 100644 --- a/src/vs/workbench/api/common/extHost.protocol.ts +++ b/src/vs/workbench/api/common/extHost.protocol.ts @@ -51,7 +51,7 @@ import { SaveReason } from 'vs/workbench/common/editor'; import { IRevealOptions, ITreeItem, IViewBadge } from 'vs/workbench/common/views'; import { CallHierarchyItem } from 'vs/workbench/contrib/callHierarchy/common/callHierarchy'; import { DebugConfigurationProviderTriggerKind, IAdapterDescriptor, IConfig, IDebugSessionReplMode } from 'vs/workbench/contrib/debug/common/debug'; -import { IInteractiveSessionResponseCommandFollowup } from 'vs/workbench/contrib/interactiveSession/common/interactiveSessionModel'; +import { IInteractiveResponseErrorDetails, IInteractiveSessionResponseCommandFollowup } from 'vs/workbench/contrib/interactiveSession/common/interactiveSessionModel'; import * as notebookCommon from 'vs/workbench/contrib/notebook/common/notebookCommon'; import { CellExecutionUpdateType } from 'vs/workbench/contrib/notebook/common/notebookExecutionService'; import { ICellExecutionComplete, ICellExecutionStateUpdate } from 'vs/workbench/contrib/notebook/common/notebookExecutionStateService'; @@ -1102,6 +1102,7 @@ export interface IInteractiveRequestDto { export interface IInteractiveResponseDto { followups?: string[]; commandFollowups?: IInteractiveSessionResponseCommandFollowup[]; + errorDetails?: IInteractiveResponseErrorDetails; } export interface IInteractiveResponseProgressDto { diff --git a/src/vs/workbench/api/common/extHostInteractiveSession.ts b/src/vs/workbench/api/common/extHostInteractiveSession.ts index b61bbfe1f63..efab94bc0e4 100644 --- a/src/vs/workbench/api/common/extHostInteractiveSession.ts +++ b/src/vs/workbench/api/common/extHostInteractiveSession.ts @@ -6,6 +6,7 @@ import { CancellationToken } from 'vs/base/common/cancellation'; import { toDisposable } from 'vs/base/common/lifecycle'; import { withNullAsUndefined } from 'vs/base/common/types'; +import { localize } from 'vs/nls'; import { IRelaxedExtensionDescription } from 'vs/platform/extensions/common/extensions'; import { ILogService } from 'vs/platform/log/common/log'; import { ExtHostInteractiveSessionShape, IInteractiveRequestDto, IInteractiveResponseDto, IInteractiveSessionDto, IMainContext, MainContext, MainThreadInteractiveSessionShape } from 'vs/workbench/api/common/extHost.protocol'; @@ -33,7 +34,7 @@ export class ExtHostInteractiveSession implements ExtHostInteractiveSessionShape constructor( mainContext: IMainContext, - _logService: ILogService + private readonly logService: ILogService ) { this._proxy = mainContext.getProxy(MainContext.MainThreadInteractiveSession); } @@ -147,20 +148,30 @@ export class ExtHostInteractiveSession implements ExtHostInteractiveSessionShape const progressObj: vscode.Progress = { report: (progress: vscode.InteractiveProgress) => this._proxy.$acceptInteractiveResponseProgress(handle, sessionId, { responsePart: progress.content }) }; - const res = await entry.provider.provideResponseWithProgress(requestObj, progressObj, token); - if (realSession.saveState) { - const newState = realSession.saveState(); - this._proxy.$acceptInteractiveSessionState(sessionId, newState); + let result: vscode.InteractiveResponseForProgress | undefined | null; + try { + result = await entry.provider.provideResponseWithProgress(requestObj, progressObj, token); + if (!result) { + result = { errorDetails: { message: localize('emptyResponse', "Provider returned null response") } }; + } + } catch (err) { + result = { errorDetails: { message: localize('errorResponse', "Error from provider: {0}", err.message) } }; + this.logService.error(err); } - if (!res) { - return; + try { + if (realSession.saveState) { + const newState = realSession.saveState(); + this._proxy.$acceptInteractiveSessionState(sessionId, newState); + } + } catch (err) { + this.logService.warn(err); } - return { followups: res.followups, commandFollowups: res.commands }; + return { followups: result.followups, commandFollowups: result.commands, errorDetails: result.errorDetails }; } - throw new Error('provider must implement either provideResponse or provideResponseWithProgress'); + throw new Error('Provider must implement either provideResponse or provideResponseWithProgress'); } $releaseSession(sessionId: number) { diff --git a/src/vs/workbench/contrib/interactiveSession/browser/interactiveSessionListRenderer.ts b/src/vs/workbench/contrib/interactiveSession/browser/interactiveSessionListRenderer.ts index e49ad49c600..2ed934268c4 100644 --- a/src/vs/workbench/contrib/interactiveSession/browser/interactiveSessionListRenderer.ts +++ b/src/vs/workbench/contrib/interactiveSession/browser/interactiveSessionListRenderer.ts @@ -5,6 +5,7 @@ import * as dom from 'vs/base/browser/dom'; import { Button } from 'vs/base/browser/ui/button/button'; +import { renderIcon } from 'vs/base/browser/ui/iconLabel/iconLabels'; import { IListVirtualDelegate } from 'vs/base/browser/ui/list/list'; import { IListAccessibilityProvider } from 'vs/base/browser/ui/list/listWidget'; import { ITreeNode, ITreeRenderer } from 'vs/base/browser/ui/tree/tree'; @@ -60,7 +61,7 @@ interface IItemHeightChangeParams { height: number; } -const forceVerboseLayoutTracing = true; +const forceVerboseLayoutTracing = false; export class InteractiveListItemRenderer extends Disposable implements ITreeRenderer { static readonly cursorCharacter = '\u258c'; @@ -176,12 +177,12 @@ export class InteractiveListItemRenderer extends Disposable implements ITreeRend this.traceLayout('renderElement', `start progressive render ${kind}, index=${index}`); const progressiveRenderingDisposables = templateData.elementDisposables.add(new DisposableStore()); const timer = templateData.elementDisposables.add(new IntervalTimer()); - const runProgressiveRender = () => { - if (this.doNextProgressiveRender(element, index, templateData, progressiveRenderingDisposables)) { + const runProgressiveRender = (initial?: boolean) => { + if (this.doNextProgressiveRender(element, index, templateData, !!initial, progressiveRenderingDisposables)) { timer.cancel(); } }; - runProgressiveRender(); + runProgressiveRender(true); timer.cancelAndSet(runProgressiveRender, 100); } else if (isResponseVM(element)) { this.basicRenderElement(element.response.value, element, index, templateData); @@ -196,6 +197,11 @@ export class InteractiveListItemRenderer extends Disposable implements ITreeRend templateData.value.appendChild(result.element); templateData.elementDisposables.add(result); + if (isResponseVM(element) && element.errorDetails) { + const errorDetails = dom.append(templateData.value, $('.interactive-response-error-details', undefined, renderIcon(Codicon.error))); + errorDetails.appendChild($('span', undefined, element.errorDetails.message)); + } + if (isResponseVM(element) && index === this.delegate.getListLength() - 1) { const followupsContainer = dom.append(templateData.value, $('.interactive-response-followups')); const followups = element.commandFollowups ?? element.followups ?? []; @@ -217,36 +223,43 @@ export class InteractiveListItemRenderer extends Disposable implements ITreeRend } } - private doNextProgressiveRender(element: IInteractiveResponseViewModel, index: number, templateData: IInteractiveListItemTemplate, disposables: DisposableStore): boolean { + private doNextProgressiveRender(element: IInteractiveResponseViewModel, index: number, templateData: IInteractiveListItemTemplate, isInRenderElement: boolean, disposables: DisposableStore): boolean { disposables.clear(); - const toRender = this.getProgressiveMarkdownToRender(element); - if (toRender) { - const isFullyRendered = element.renderData?.isFullyRendered; - if (isFullyRendered) { - this.traceLayout('runProgressiveRender', `end progressive render, index=${index}`); - if (element.isComplete) { - this.traceLayout('runProgressiveRender', `and disposing renderData, response is complete, index=${index}`); - element.renderData = undefined; - } else { - this.traceLayout('runProgressiveRender', `Rendered all available words, but model is not complete.`); - } - disposables.clear(); - this.basicRenderElement(element.response.value, element, index, templateData); - } else { - const plusCursor = toRender.match(/```.*$/) ? toRender + `\n${InteractiveListItemRenderer.cursorCharacter}` : toRender + ` ${InteractiveListItemRenderer.cursorCharacter}`; - const result = this.renderMarkdown(element, index, new MarkdownString(plusCursor), disposables, templateData, true); - dom.clearNode(templateData.value); - templateData.value.appendChild(result.element); - disposables.add(result); - } - const height = templateData.rowContainer.offsetHeight; - element.currentRenderedHeight = height; - this._onDidChangeItemHeight.fire({ element, height: templateData.rowContainer.offsetHeight }); - return !!isFullyRendered; + // TODO- this method has the side effect of updating element.renderData + const toRender = this.getProgressiveMarkdownToRender(element); + const isFullyRendered = element.renderData?.isFullyRendered; + if (isFullyRendered) { + // We've reached the end of the available content, so do a normal render + this.traceLayout('runProgressiveRender', `end progressive render, index=${index}`); + if (element.isComplete) { + this.traceLayout('runProgressiveRender', `and disposing renderData, response is complete, index=${index}`); + element.renderData = undefined; + } else { + this.traceLayout('runProgressiveRender', `Rendered all available words, but model is not complete.`); + } + disposables.clear(); + this.basicRenderElement(element.response.value, element, index, templateData); + } else if (toRender) { + // Doing the progressive render + const plusCursor = toRender.match(/```.*$/) ? toRender + `\n${InteractiveListItemRenderer.cursorCharacter}` : toRender + ` ${InteractiveListItemRenderer.cursorCharacter}`; + const result = this.renderMarkdown(element, index, new MarkdownString(plusCursor), disposables, templateData, true); + dom.clearNode(templateData.value); + templateData.value.appendChild(result.element); + disposables.add(result); + } else { + // Nothing new to render, not done, keep waiting + return false; } - return false; + // Some render happened - update the height + const height = templateData.rowContainer.offsetHeight; + element.currentRenderedHeight = height; + if (!isInRenderElement) { + this._onDidChangeItemHeight.fire({ element, height: templateData.rowContainer.offsetHeight }); + } + + return !!isFullyRendered; } private renderMarkdown(element: InteractiveTreeItem, index: number, markdown: IMarkdownString, disposables: DisposableStore, templateData: IInteractiveListItemTemplate, fillInIncompleteTokens = false): IMarkdownRenderResult { diff --git a/src/vs/workbench/contrib/interactiveSession/browser/media/interactiveSession.css b/src/vs/workbench/contrib/interactiveSession/browser/media/interactiveSession.css index b8123e6aff3..f0bf04a10cd 100644 --- a/src/vs/workbench/contrib/interactiveSession/browser/media/interactiveSession.css +++ b/src/vs/workbench/contrib/interactiveSession/browser/media/interactiveSession.css @@ -184,3 +184,13 @@ font-size: 11px; font-weight: 600; } + +.interactive-session .interactive-response .interactive-response-error-details { + display: flex; + align-items: center; + gap: 6px; +} + +.interactive-session .interactive-response .interactive-response-error-details .codicon { + color: var(--vscode-errorForeground); +} diff --git a/src/vs/workbench/contrib/interactiveSession/common/interactiveSessionModel.ts b/src/vs/workbench/contrib/interactiveSession/common/interactiveSessionModel.ts index e522e7b95ea..215c90aee77 100644 --- a/src/vs/workbench/contrib/interactiveSession/common/interactiveSessionModel.ts +++ b/src/vs/workbench/contrib/interactiveSession/common/interactiveSessionModel.ts @@ -8,7 +8,7 @@ import { IMarkdownString, MarkdownString } from 'vs/base/common/htmlContent'; import { Disposable } from 'vs/base/common/lifecycle'; import { URI } from 'vs/base/common/uri'; import { ILogService } from 'vs/platform/log/common/log'; -import { IInteractiveSession } from 'vs/workbench/contrib/interactiveSession/common/interactiveSessionService'; +import { IInteractiveResponse, IInteractiveSession } from 'vs/workbench/contrib/interactiveSession/common/interactiveSessionService'; export interface IInteractiveRequestModel { readonly id: string; @@ -24,6 +24,11 @@ export interface IInteractiveSessionResponseCommandFollowup { title: string; // supports codicon strings } +export interface IInteractiveResponseErrorDetails { + message: string; + responseIsIncomplete?: boolean; +} + export interface IInteractiveResponseModel { readonly onDidChange: Event; readonly id: string; @@ -33,6 +38,7 @@ export interface IInteractiveResponseModel { readonly isComplete: boolean; readonly followups?: string[]; readonly commandFollowups?: IInteractiveSessionResponseCommandFollowup[]; + readonly errorDetails?: IInteractiveResponseErrorDetails; } export function isRequest(item: unknown): item is IInteractiveRequestModel { @@ -89,11 +95,17 @@ export class InteractiveResponseModel extends Disposable implements IInteractive return this._response; } - constructor(response: IMarkdownString, public readonly username: string, public readonly avatarIconUri?: URI, isComplete: boolean = false, followups?: string[]) { + private _errorDetails: IInteractiveResponseErrorDetails | undefined; + public get errorDetails(): IInteractiveResponseErrorDetails | undefined { + return this._errorDetails; + } + + constructor(response: IMarkdownString, public readonly username: string, public readonly avatarIconUri?: URI, isComplete: boolean = false, errorDetails?: IInteractiveResponseErrorDetails, followups?: string[]) { super(); this._response = response; this._isComplete = isComplete; this._followups = followups; + this._errorDetails = errorDetails; this._id = 'response_' + InteractiveResponseModel.nextId++; } @@ -102,10 +114,11 @@ export class InteractiveResponseModel extends Disposable implements IInteractive this._onDidChange.fire(); } - complete(followups: string[] | undefined, commandFollowups: IInteractiveSessionResponseCommandFollowup[] | undefined): void { + complete(followups: string[] | undefined, commandFollowups: IInteractiveSessionResponseCommandFollowup[] | undefined, errorDetails?: IInteractiveResponseErrorDetails): void { this._isComplete = true; this._followups = followups; this._commandFollowups = commandFollowups; + this._errorDetails = errorDetails; this._onDidChange.fire(); } } @@ -125,6 +138,7 @@ export interface ISerializableInteractiveSessionsData { export interface ISerializableInteractiveSessionRequestData { message: string; response: string | undefined; + responseErrorDetails: IInteractiveResponseErrorDetails | undefined; } export interface ISerializableInteractiveSessionData { @@ -181,10 +195,10 @@ export class InteractiveSessionModel extends Disposable implements IInteractiveS return []; } - return requests.map((r: ISerializableInteractiveSessionRequestData) => { - const request = new InteractiveRequestModel(r.message, this.session.requesterUsername, this.session.requesterAvatarIconUri); - if (r.response) { - request.response = new InteractiveResponseModel(new MarkdownString(r.response), this.session.responderUsername, this.session.responderAvatarIconUri, true); + return requests.map((raw: ISerializableInteractiveSessionRequestData) => { + const request = new InteractiveRequestModel(raw.message, this.session.requesterUsername, this.session.requesterAvatarIconUri); + if (raw.response || raw.responseErrorDetails) { + request.response = new InteractiveResponseModel(new MarkdownString(raw.response), this.session.responderUsername, this.session.responderAvatarIconUri, true, raw.responseErrorDetails); } return request; }); @@ -224,8 +238,8 @@ export class InteractiveSessionModel extends Disposable implements IInteractiveS } } - completeResponse(request: InteractiveRequestModel, followups?: string[], commandFollowups?: IInteractiveSessionResponseCommandFollowup[]): void { - request.response!.complete(followups, commandFollowups); + completeResponse(request: InteractiveRequestModel, response: IInteractiveResponse): void { + request.response!.complete(response.followups, response.commandFollowups, response.errorDetails); } setResponse(request: InteractiveRequestModel, response: InteractiveResponseModel): void { @@ -239,6 +253,7 @@ export class InteractiveSessionModel extends Disposable implements IInteractiveS return { message: r.message, response: r.response ? r.response.response.value : undefined, + responseErrorDetails: r.response?.errorDetails }; }), providerId: this.providerId, diff --git a/src/vs/workbench/contrib/interactiveSession/common/interactiveSessionService.ts b/src/vs/workbench/contrib/interactiveSession/common/interactiveSessionService.ts index 1f337e7d7bf..de8a9584372 100644 --- a/src/vs/workbench/contrib/interactiveSession/common/interactiveSessionService.ts +++ b/src/vs/workbench/contrib/interactiveSession/common/interactiveSessionService.ts @@ -8,7 +8,7 @@ import { IDisposable } from 'vs/base/common/lifecycle'; import { URI } from 'vs/base/common/uri'; import { ProviderResult } from 'vs/editor/common/languages'; import { createDecorator } from 'vs/platform/instantiation/common/instantiation'; -import { IInteractiveSessionResponseCommandFollowup, InteractiveSessionModel } from 'vs/workbench/contrib/interactiveSession/common/interactiveSessionModel'; +import { IInteractiveResponseErrorDetails, IInteractiveSessionResponseCommandFollowup, InteractiveSessionModel } from 'vs/workbench/contrib/interactiveSession/common/interactiveSessionModel'; export interface IInteractiveSession { id: number; @@ -28,6 +28,7 @@ export interface IInteractiveResponse { session: IInteractiveSession; followups?: string[]; commandFollowups?: IInteractiveSessionResponseCommandFollowup[]; + errorDetails?: IInteractiveResponseErrorDetails; } export interface IInteractiveProgress { diff --git a/src/vs/workbench/contrib/interactiveSession/common/interactiveSessionServiceImpl.ts b/src/vs/workbench/contrib/interactiveSession/common/interactiveSessionServiceImpl.ts index d6a295fcabe..bd1e34bd48e 100644 --- a/src/vs/workbench/contrib/interactiveSession/common/interactiveSessionServiceImpl.ts +++ b/src/vs/workbench/contrib/interactiveSession/common/interactiveSessionServiceImpl.ts @@ -8,6 +8,7 @@ import { groupBy } from 'vs/base/common/collections'; import { Iterable } from 'vs/base/common/iterator'; import { Disposable, IDisposable, toDisposable } from 'vs/base/common/lifecycle'; import { withNullAsUndefined } from 'vs/base/common/types'; +import { localize } from 'vs/nls'; import { IInstantiationService } from 'vs/platform/instantiation/common/instantiation'; import { ILogService } from 'vs/platform/log/common/log'; import { IStorageService, StorageScope, StorageTarget } from 'vs/platform/storage/common/storage'; @@ -125,7 +126,6 @@ export class InteractiveSessionService extends Disposable implements IInteractiv return false; } - // TODO log failures, add dummy response with error message const _sendRequest = async (): Promise => { try { this._pendingRequestSessions.add(sessionId); @@ -134,13 +134,13 @@ export class InteractiveSessionService extends Disposable implements IInteractiv this.trace('sendRequest', `Provider returned progress for session ${sessionId}, ${progress.responsePart.length} chars`); model.mergeResponseContent(request, progress.responsePart); }; - const rawResponse = await provider.provideReply({ session: model.session, message }, progressCallback, token); + let rawResponse = await provider.provideReply({ session: model.session, message }, progressCallback, token); if (!rawResponse) { this.trace('sendRequest', `Provider returned no response for session ${sessionId}`); - return; + rawResponse = { session: model.session, errorDetails: { message: localize('emptyResponse', "Provider returned null response") } }; } - model.completeResponse(request, rawResponse.followups, rawResponse.commandFollowups); + model.completeResponse(request, rawResponse); this.trace('sendRequest', `Provider returned response for session ${sessionId} with ${rawResponse.followups} followups`); } finally { this._pendingRequestSessions.delete(sessionId); diff --git a/src/vs/workbench/contrib/interactiveSession/common/interactiveSessionViewModel.ts b/src/vs/workbench/contrib/interactiveSession/common/interactiveSessionViewModel.ts index 4e96a75f100..a2fee1ab162 100644 --- a/src/vs/workbench/contrib/interactiveSession/common/interactiveSessionViewModel.ts +++ b/src/vs/workbench/contrib/interactiveSession/common/interactiveSessionViewModel.ts @@ -9,7 +9,7 @@ import { Disposable } from 'vs/base/common/lifecycle'; import { URI } from 'vs/base/common/uri'; import { IInstantiationService } from 'vs/platform/instantiation/common/instantiation'; import { ILogService } from 'vs/platform/log/common/log'; -import { IInteractiveRequestModel, IInteractiveResponseModel, IInteractiveSessionModel, IInteractiveSessionResponseCommandFollowup } from 'vs/workbench/contrib/interactiveSession/common/interactiveSessionModel'; +import { IInteractiveRequestModel, IInteractiveResponseErrorDetails, IInteractiveResponseModel, IInteractiveSessionModel, IInteractiveSessionResponseCommandFollowup } from 'vs/workbench/contrib/interactiveSession/common/interactiveSessionModel'; import { IInteractiveSessionService } from 'vs/workbench/contrib/interactiveSession/common/interactiveSessionService'; import { countWords } from 'vs/workbench/contrib/interactiveSession/common/interactiveSessionWordCounter'; @@ -58,6 +58,7 @@ export interface IInteractiveResponseViewModel { readonly isComplete: boolean; readonly followups?: string[]; readonly commandFollowups?: IInteractiveSessionResponseCommandFollowup[]; + readonly errorDetails?: IInteractiveResponseErrorDetails; readonly progressiveResponseRenderingEnabled: boolean; readonly contentUpdateTimings?: IInteractiveSessionLiveUpdateData; renderData?: IInteractiveResponseRenderData; @@ -196,6 +197,10 @@ export class InteractiveResponseViewModel extends Disposable implements IInterac return this._model.commandFollowups; } + get errorDetails() { + return this._model.errorDetails; + } + renderData: IInteractiveResponseRenderData | undefined = undefined; currentRenderedHeight: number | undefined; @@ -224,7 +229,7 @@ export class InteractiveResponseViewModel extends Disposable implements IInterac } this._register(_model.onDidChange(() => { - if (this._isPlaceholder && _model.response.value) { + if (this._isPlaceholder && (_model.response.value || this.isComplete)) { this._isPlaceholder = false; if (this.renderData) { this.renderData.renderedWordCount = 0; diff --git a/src/vscode-dts/vscode.proposed.interactive.d.ts b/src/vscode-dts/vscode.proposed.interactive.d.ts index df8ce2a6a89..901fb1bebdf 100644 --- a/src/vscode-dts/vscode.proposed.interactive.d.ts +++ b/src/vscode-dts/vscode.proposed.interactive.d.ts @@ -76,9 +76,15 @@ declare module 'vscode' { followups?: string[]; } + export interface InteractiveResponseErrorDetails { + message: string; + responseIsIncomplete?: boolean; + } + export interface InteractiveResponseForProgress { followups?: string[]; commands?: InteractiveResponseCommand[]; + errorDetails?: InteractiveResponseErrorDetails; } export interface InteractiveProgress {