Skip to content

Commit 2da00ee

Browse files
committed
fix error messages content for both editor and in-chat attachment cases
1 parent 92de76d commit 2da00ee

File tree

7 files changed

+226
-212
lines changed

7 files changed

+226
-212
lines changed

src/vs/workbench/contrib/chat/browser/attachments/promptAttachments/promptAttachmentWidget.ts

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -22,8 +22,8 @@ import { ILanguageService } from '../../../../../../editor/common/languages/lang
2222
import { FileKind, IFileService } from '../../../../../../platform/files/common/files.js';
2323
import { IMenuService, MenuId } from '../../../../../../platform/actions/common/actions.js';
2424
import { getCleanPromptName } from '../../../../../../platform/prompts/common/constants.js';
25-
import { ChatPromptAttachmentModel } from '../../chatAttachmentModel/chatPromptAttachmentModel.js';
2625
import { IContextKeyService } from '../../../../../../platform/contextkey/common/contextkey.js';
26+
import { ChatPromptAttachmentModel } from '../../chatAttachmentModel/chatPromptAttachmentModel.js';
2727
import { IContextMenuService } from '../../../../../../platform/contextview/browser/contextView.js';
2828
import { getDefaultHoverDelegate } from '../../../../../../base/browser/ui/hover/hoverDelegateFactory.js';
2929
import { getFlatContextMenuActions } from '../../../../../../platform/actions/browser/menuEntryActionViewItem.js';
@@ -118,18 +118,18 @@ export class PromptAttachmentWidget extends Disposable {
118118
// add the issue details in the hover title for the attachment, one
119119
// error/warning at a time because there is a limited space available
120120
if (topError) {
121-
const { isRootError, localizedMessage: details } = topError;
122-
const isWarning = !isRootError;
121+
const { errorSubject: subject } = topError;
122+
const isError = (subject === 'root');
123123

124124
this.domNode.classList.add(
125-
(isWarning) ? 'warning' : 'error',
125+
(isError) ? 'error' : 'warning',
126126
);
127127

128-
const errorCaption = (isWarning)
129-
? localize('warning', "Warning")
130-
: localize('error', "Error");
128+
const severity = (isError)
129+
? localize('error', "Error")
130+
: localize('warning', "Warning");
131131

132-
title += `\n-\n[${errorCaption}]: ${details}`;
132+
title += `\n[${severity}]: ${topError.localizedMessage}`;
133133
}
134134

135135
const fileWithoutExtension = getCleanPromptName(file);

src/vs/workbench/contrib/chat/common/promptFileReferenceErrors.ts

Lines changed: 48 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,8 @@
44
*--------------------------------------------------------------------------------------------*/
55

66
import { URI } from '../../../../base/common/uri.js';
7+
import { basename } from '../../../../base/common/path.js';
8+
import { assert, assertNever } from '../../../../base/common/assert.js';
79

810
/**
911
* Base prompt parsing error class.
@@ -89,6 +91,12 @@ export class OpenFailed extends FailedToResolveContentsStream {
8991
}
9092
}
9193

94+
/**
95+
* Character use to join filenames/paths in a chain of references that
96+
* lead to recursion.
97+
*/
98+
const DEFAULT_RECURSIVE_PATH_JOIN_CHAR = ' -> ';
99+
92100
/**
93101
* Error that reflects the case when attempt resolve nested file
94102
* references failes due to a recursive reference, e.g.,
@@ -106,23 +114,53 @@ export class OpenFailed extends FailedToResolveContentsStream {
106114
export class RecursiveReference extends ResolveError {
107115
public override errorType = 'RecursiveReferenceError';
108116

117+
/**
118+
* Default string representation of the recursive path.
119+
*/
120+
public readonly recursivePathString: string;
121+
109122
constructor(
110123
uri: URI,
111124
public readonly recursivePath: string[],
112125
) {
113-
const references = recursivePath.join(' -> ');
126+
// sanity check - a recursive path must always have at least
127+
// two items in the list, otherwise it is not a recursive loop
128+
assert(
129+
recursivePath.length >= 2,
130+
`Recursive path must contain at least two paths, got '${recursivePath.length}'.`,
131+
);
114132

133+
const pathString = recursivePath.join(DEFAULT_RECURSIVE_PATH_JOIN_CHAR);
115134
super(
116135
uri,
117-
`Recursive references found: ${references}.`,
136+
`Recursive references found: ${pathString}.`,
118137
);
138+
this.recursivePathString = pathString;
119139
}
120140

121141
/**
122142
* Returns a string representation of the recursive path.
123143
*/
124-
public get recursivePathString(): string {
125-
return this.recursivePath.join(' -> ');
144+
public getRecursivePathString(
145+
filename: 'basename' | 'fullpath',
146+
pathJoinCharacter: string = DEFAULT_RECURSIVE_PATH_JOIN_CHAR,
147+
): string {
148+
return this.recursivePath
149+
.map((path) => {
150+
if (filename === 'fullpath') {
151+
return `'${path}'`;
152+
}
153+
154+
if (filename === 'basename') {
155+
return `'${basename(path)}'`;
156+
}
157+
158+
assertNever(
159+
filename,
160+
`Unknown filename format '${filename}'.`,
161+
);
162+
})
163+
.join(pathJoinCharacter);
126164
}
127165

128166
/**
@@ -138,6 +176,12 @@ export class RecursiveReference extends ResolveError {
138176
return false;
139177
}
140178

179+
// performance optimization - if the paths lengths don't match,
180+
// no need to compare entire strings as they must be different
181+
if (this.recursivePathString.length !== other.recursivePathString.length) {
182+
return false;
183+
}
184+
141185
return this.recursivePathString === other.recursivePathString;
142186
}
143187

src/vs/workbench/contrib/chat/common/promptSyntax/contentProviders/types.d.ts

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@
44
*--------------------------------------------------------------------------------------------*/
55

66
import { URI } from '../../../../../../base/common/uri.js';
7-
import { ParseError } from '../../promptFileReferenceErrors.js';
7+
import { ResolveError } from '../../promptFileReferenceErrors.js';
88
import { IDisposable } from '../../../../../../base/common/lifecycle.js';
99
import { VSBufferReadableStream } from '../../../../../../base/common/buffer.js';
1010

@@ -27,9 +27,9 @@ export interface IPromptContentsProvider extends IDisposable {
2727
/**
2828
* Event that fires when the prompt contents change. The event is either a
2929
* {@linkcode VSBufferReadableStream} stream with changed contents or
30-
* an instance of the {@linkcode ParseError} error.
30+
* an instance of the {@linkcode ResolveError} error.
3131
*/
3232
onContentChanged(
33-
callback: (streamOrError: VSBufferReadableStream | ParseError) => void,
33+
callback: (streamOrError: VSBufferReadableStream | ResolveError) => void,
3434
): IDisposable;
3535
}

src/vs/workbench/contrib/chat/common/promptSyntax/languageFeatures/promptLinkDiagnosticsProvider.ts

Lines changed: 2 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -24,11 +24,6 @@ import { IInstantiationService } from '../../../../../../platform/instantiation/
2424
import { IConfigurationService } from '../../../../../../platform/configuration/common/configuration.js';
2525
import { IMarkerData, IMarkerService, MarkerSeverity } from '../../../../../../platform/markers/common/markers.js';
2626

27-
/**
28-
* TODO: @legomushroom - list
29-
* - improve error messages
30-
*/
31-
3227
/**
3328
* TODO: @legomushroom
3429
*/
@@ -124,9 +119,8 @@ const toMarker = (
124119
'Error must not be of "not prompt file" type.',
125120
);
126121

127-
// use `error` severity if the error relates to the link itself, and use
128-
// the `warning` severity if the error is related to one of its children
129-
const severity = (topError.isRootError)
122+
// `error` severity for the link itself, `warning` for any of its children
123+
const severity = (topError.errorSubject === 'root')
130124
? MarkerSeverity.Error
131125
: MarkerSeverity.Warning;
132126

0 commit comments

Comments
 (0)